[PATCH RFC v2 00/16] drm/vkms: ConfigFS interface

[PATCH RFC v2 00/16] drm/vkms: ConfigFS interface

[Louis Chauvet]

VKMS is manly used to test userspace program and its behavior. The current 
implementation is not very configurable as you can only have one device, 
with few specific planes.

This series aims to introduce a new interface, using ConfigFS, to create 
and configure more devices. This will introduce:
- Device creation
- Plane creation
- Plane configuration (type, color encoding, color range, rotations)
- Encoder creation
- CRTC creation
- Linking between CRTC and planes/encoders

The proposition is:
/config/vkms
	DEVICE_1
	┣━ enable
	┣━ writeback
	┣━ planes
	┃  ┣━ PLANE_1
	┃  ┃  ┣━ type
	┃  ┃  ┣━ supported_rotations
	┃  ┃  ┣━ color_range
	┃  ┃  ┣━ color_encoding
	┃  ┃  ┣━ default_color_encoding
	┃  ┃  ┣━ default_rotations
	┃  ┃  ┣━ default_color_range
	┃  ┃  ┗━ possible_crtcs
	┃  ┃     ┗━ >> /config/vkms/DEVICE_1/crtc/CRTC_1 
	┃  ┣━ PLANE_2
	┃  ┃  ┗━ ditto
	┃  ┗━ PLANE_3
	┃     ┗━ ditto
	┃
	┣━ encoders
	┃  ┣━ ENCODER_1
	┃  ┃  ┗━ possible_crtcs
	┃  ┃     ┗━ >> /config/vkms/DEVICE_1/crtc/CRTC_1
	┃  ┗━ ENCODER_2
	┃     ┗━ ditto
	┃
	┗━ crtc
	   ┗━ CRTC_1
	
This interface aims to be extendable (new property can easly be added in 
objects) and easy to use (objects are created simply by creating folders, 
and configured by writing files).

This series depends on 
https://lore.kernel.org/all/20241122-google-remove-crtc-index-from-parameter-v2-0-81540742535a@bootlin.com
but as this is a bit complex to rebase, you can find a working branch 
here:
https://gitlab.freedesktop.org/louischauvet/kernel/-/tree/b4/vkms-configfs

Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
---
Changes in v2:
- Added many new configuration (mainly connector)
- Link to v1: https://lore.kernel.org/r/20240814-google-config-fs-v1-0-8363181907a6@bootlin.com

---
Louis Chauvet (16):
      drm/vkms: Add vkms_delete/create_device helper
      drm/vkms: Cleanup configuration field on device destroy
      drm/vkms: Introduce ConfigFS interface
      drm/vkms: Introduce configfs for plane
      drm/vkms: Introduce configfs for plane rotation
      drm/vkms: Introduce configfs for plane color encoding
      drm/vkms: Introduce configfs for plane color range
      drm/vkms: Introduce configfs for crtc and encoder
      drm/vkms: Introduce configfs for connectors
      drm/vkms: Introduce configfs for connector type
      drm/vkms: Introduce configfs for plane format
      drm/vkms: Introduce configfs for device name
      drm/vkms: Introduce configfs for connector status
      drm/vkms: Introduce configfs for connector id
      drm/vkms: Introduce configfs for connector EDID
      drm/vkms: Introduce configfs for encoder type

 drivers/gpu/drm/vkms/Kconfig         |    1 +
 drivers/gpu/drm/vkms/Makefile        |    1 +
 drivers/gpu/drm/vkms/vkms_config.c   |   36 +
 drivers/gpu/drm/vkms/vkms_config.h   |    6 +-
 drivers/gpu/drm/vkms/vkms_configfs.c | 1404 ++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/vkms/vkms_configfs.h |  128 ++++
 drivers/gpu/drm/vkms/vkms_drv.c      |   21 +-
 drivers/gpu/drm/vkms/vkms_drv.h      |    3 +
 8 files changed, 1594 insertions(+), 6 deletions(-)
---
base-commit: 98efdd02e220fea84c1491012d7292749a71faeb
change-id: 20240521-google-config-fs-b935b66b8d7b
prerequisite-message-id: 20241122-google-vkms-managed-v5-0-1ab60403e960@bootlin.com
prerequisite-patch-id: b608594ad493a41000ee703792eac4b23f9e35dc
prerequisite-patch-id: 5697aa87c44bbf3eda8a1ba424465dc792545d4c
prerequisite-patch-id: 223d59c407ce28dacf3f563b5c0148d2398303f1
prerequisite-patch-id: 720b75b21d06ce3d3f060fb9238f7903834da0e1
prerequisite-patch-id: 30a1e033fa43241ca6a43006fd4f29f8e9217224
prerequisite-message-id: 20241122-b4-vkms-allocated-v2-0-ff7bddbf0bfb@bootlin.com
prerequisite-patch-id: 9741873a5f0a7a3cf117dec7837354c3ad38ac3a
prerequisite-patch-id: 1a383d1494e4f2142b62822f2ba482a3b813563a
prerequisite-patch-id: 7d3f49fee4d3553d52fc075b7868da9dea9209cd
prerequisite-patch-id: 57f5aeff2a9e8f2b6f47569e44dcd8fa587ed4bf
prerequisite-message-id: 20241122-b4-new-color-formats-v3-0-23f7776197c9@bootlin.com
prerequisite-patch-id: e6717b75d79ae5cfb0815bab88d722082107dc0e
prerequisite-patch-id: 4b3b1ea5ad2e3ba1922cd4b3d3d46214b27c8c2d
prerequisite-patch-id: 060874d5a7433cc8cc654bc63e0b411036727ebb
prerequisite-patch-id: 43115d21842e508d9d8b0468e15f67d442bffe3c
prerequisite-patch-id: 627d0970e76d4154c982d0d4172e7a0c4dfb9a4c
prerequisite-patch-id: 582445144ac0ab11175ef96262060b08a5e1467e
prerequisite-patch-id: a98fac5a2c60fe23fbc6a455e9a4ab8b0f187ee8
prerequisite-patch-id: 62c8d109a22b9978f755255b67f13fe74fb7008d
prerequisite-message-id: 20241122-writeback_line_by_line-v3-0-085d5810f6e3@bootlin.com
prerequisite-patch-id: 07868dd9c7bbb1ed96d675c689de86f0cf293248
prerequisite-patch-id: 736638b76050ef7a99cfad2c1560f7af114d5fbd
prerequisite-patch-id: 20d8823f9c1d372ab2b88f969f5110f77e49c7f9
prerequisite-message-id: 20241122-google-remove-crtc-index-from-parameter-v2-0-81540742535a@bootlin.com
prerequisite-patch-id: a71faf4da3bf2d2e3e8744d9ca688d7ad47d86a1
prerequisite-patch-id: aefc656e8bc9edfd5527971231b7db23b416a19a
prerequisite-patch-id: f17cbbe9923f78e4d5fdce56d0fb3c3af1fa3b82
prerequisite-patch-id: d7a7e404fcb1656d722b59ef13da771e6156916f
prerequisite-patch-id: f0d5640738b5947ab84272c458a2f729a611ab0f
prerequisite-patch-id: 26b9a61db21be516a2c84072a71f2c11d21a828d
prerequisite-patch-id: 93571d6f80d345fa5d705d8a677bca183852554f
prerequisite-patch-id: 9c8bee631c4a466b195adbfadb62eb15fa371cce
prerequisite-patch-id: b098550055ebeabd85bc5a5ea7c9aae2a52a4ac8
prerequisite-patch-id: 83a4752516471b5152d5e2cb6b56d7ecaae70f66
prerequisite-patch-id: f02ce6f3ffcb94f955f7b51f6d2120b93cc9f76e
prerequisite-patch-id: 75a8bb8ced4d23de2de8b5355b395171a7782981
prerequisite-patch-id: a6b7a6442de27a3815985a1857ae6eceecc3687a
prerequisite-patch-id: c47a619ac3b9b244375776aa03b6c19c32108cca
prerequisite-patch-id: 4a5efd82cc55ea5897e8bffad0d12f22e4ee3fd7
prerequisite-patch-id: c0de37e1ddbb9c040820a341284cb5814ceb4d71
prerequisite-patch-id: c87e20092e0a2f6d657837dbf1611b9dbf100b2c
prerequisite-patch-id: c12f46d01d578587d93d20e9d987394ab63a65ab

Best regards,
-- 
Louis Chauvet <louis.chauvet@bootlin.com>

[PATCH RFC v2 01/16] drm/vkms: Add vkms_delete/create_device helper

[Louis Chauvet]

In preparation for introduction of ConfigFS support, expose the
vkms_destroy/create helper to remove a vkms device.

Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
---
 drivers/gpu/drm/vkms/vkms_drv.c | 4 ++--
 drivers/gpu/drm/vkms/vkms_drv.h | 3 +++
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
index eb6fd570b4549639f6818ff63fb334f2a461b23d..2db393f5782eb26a5aa469a4774b2e19c886ee7e 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.c
+++ b/drivers/gpu/drm/vkms/vkms_drv.c
@@ -152,7 +152,7 @@ static int vkms_modeset_init(struct vkms_device *vkmsdev)
 	return vkms_output_init(vkmsdev);
 }
 
-static int vkms_create(struct vkms_config *config)
+int vkms_create(struct vkms_config *config)
 {
 	int ret;
 	struct platform_device *pdev;
@@ -226,7 +226,7 @@ static int __init vkms_init(void)
 	return ret;
 }
 
-static void vkms_destroy(struct vkms_config *config)
+void vkms_destroy(struct vkms_config *config)
 {
 	struct platform_device *pdev;
 
diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
index a772bf4168e11730c6ee2e3c79abce3a6351203f..5521d59f775170d828dea734b4ed3d177debdd0d 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.h
+++ b/drivers/gpu/drm/vkms/vkms_drv.h
@@ -231,6 +231,9 @@ struct vkms_device {
 	const struct vkms_config *config;
 };
 
+int vkms_create(struct vkms_config *config);
+void vkms_destroy(struct vkms_config *config);
+
 /*
  * The following helpers are used to convert a member of a struct into its parent.
  */

-- 
2.47.0

[PATCH RFC v2 02/16] drm/vkms: Cleanup configuration field on device destroy

[Louis Chauvet]

To avoid having dangling pointers in struct vkms_config, remove all of
them when the device is destroyed.

Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
---
 drivers/gpu/drm/vkms/vkms_config.c | 23 +++++++++++++++++++++++
 drivers/gpu/drm/vkms/vkms_config.h |  3 ++-
 drivers/gpu/drm/vkms/vkms_drv.c    |  2 +-
 3 files changed, 26 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/vkms/vkms_config.c b/drivers/gpu/drm/vkms/vkms_config.c
index 26280ad223208a978c44ef9c6c6eaadf1756818d..9a461a0481c2a20d6d48f1aa9649843ad1b7d13d 100644
--- a/drivers/gpu/drm/vkms/vkms_config.c
+++ b/drivers/gpu/drm/vkms/vkms_config.c
@@ -204,6 +204,29 @@ struct vkms_config_connector *vkms_config_create_connector(struct vkms_config *v
 	return vkms_config_connector;
 }
 
+void vkms_config_disconnect_dev(struct vkms_config *vkms_config)
+{
+	struct vkms_config_connector *connector, *tmp_connector;
+	struct vkms_config_encoder *encoder, *tmp_encoder;
+	struct vkms_config_plane *plane, *tmp_plane;
+	struct vkms_config_crtc *crtc, *tmp_crtc;
+
+	vkms_config->dev = NULL;
+
+	list_for_each_entry_safe(connector, tmp_connector, &vkms_config->connectors, link) {
+		connector->connector = NULL;
+	}
+	list_for_each_entry_safe(encoder, tmp_encoder, &vkms_config->encoders, link) {
+		encoder->encoder = NULL;
+	}
+	list_for_each_entry_safe(plane, tmp_plane, &vkms_config->planes, link) {
+		plane->plane = NULL;
+	}
+	list_for_each_entry_safe(crtc, tmp_crtc, &vkms_config->crtcs, link) {
+		crtc->crtc = NULL;
+	}
+}
+
 void vkms_config_connector_update_status(struct vkms_config_connector *vkms_config_connector,
 					 enum drm_connector_status status)
 {
diff --git a/drivers/gpu/drm/vkms/vkms_config.h b/drivers/gpu/drm/vkms/vkms_config.h
index c6fe0573abd899e4b44b5ad390ff72e12664973f..529d9c99f3c406d49dc7f3689a84c3dd775399a9 100644
--- a/drivers/gpu/drm/vkms/vkms_config.h
+++ b/drivers/gpu/drm/vkms/vkms_config.h
@@ -117,10 +117,11 @@ struct vkms_config_connector {
 	char edid_blob[PAGE_SIZE];
 	int edid_blob_len;
 
-	/* Internal usage */
+	/* Set only when the device is enabled */
 	struct drm_connector *connector;
 };
 
+void vkms_config_disconnect_dev(struct vkms_config *vkms_config);
 void vkms_config_connector_update_status(struct vkms_config_connector *vkms_config_connector,
 					 enum drm_connector_status status);
 
diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
index 2db393f5782eb26a5aa469a4774b2e19c886ee7e..8ffbd5fc65350ba03f870726b669d189f62bad6f 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.c
+++ b/drivers/gpu/drm/vkms/vkms_drv.c
@@ -242,7 +242,7 @@ void vkms_destroy(struct vkms_config *config)
 	devres_release_group(&pdev->dev, NULL);
 	platform_device_unregister(pdev);
 
-	config->dev = NULL;
+	vkms_config_disconnect_dev(config);
 }
 
 static void __exit vkms_exit(void)

-- 
2.47.0

[PATCH RFC v2 03/16] drm/vkms: Introduce ConfigFS interface

[Louis Chauvet]

VKMS is manly used to test userspace program and its behavior. The current
implementation is not very configurable as you can only have one device,
with few specific planes.

This is the introduction of a basic interface to dynamically create new
devices.

The new interface is:
/config/vkms
  DEVICE_1
  ┗━ enable
  DEVICE_2
  ┗━ enable

Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
---
 drivers/gpu/drm/vkms/Kconfig         |   1 +
 drivers/gpu/drm/vkms/Makefile        |   1 +
 drivers/gpu/drm/vkms/vkms_configfs.c | 144 +++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/vkms/vkms_configfs.h |  35 +++++++++
 drivers/gpu/drm/vkms/vkms_drv.c      |  15 +++-
 5 files changed, 194 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/vkms/Kconfig b/drivers/gpu/drm/vkms/Kconfig
index d4665e913de7702fbd5c0f047876dad9715c690a..71fe9124bf18dd5b17b0989077e1cf1f8f1ee963 100644
--- a/drivers/gpu/drm/vkms/Kconfig
+++ b/drivers/gpu/drm/vkms/Kconfig
@@ -6,6 +6,7 @@ config DRM_VKMS
 	select DRM_CLIENT_SELECTION
 	select DRM_KMS_HELPER
 	select DRM_GEM_SHMEM_HELPER
+    select CONFIGFS_FS
 	select CRC32
 	default n
 	help
diff --git a/drivers/gpu/drm/vkms/Makefile b/drivers/gpu/drm/vkms/Makefile
index 2b6db5870b977f6e5013982af89a48aec6c11983..9ecf6bc4779f81cac2677a3595c0bf4ebd1448bd 100644
--- a/drivers/gpu/drm/vkms/Makefile
+++ b/drivers/gpu/drm/vkms/Makefile
@@ -1,5 +1,6 @@
 # SPDX-License-Identifier: GPL-2.0-only
 vkms-y := \
+	vkms_configfs.o \
 	vkms_drv.o \
 	vkms_plane.o \
 	vkms_output.o \
diff --git a/drivers/gpu/drm/vkms/vkms_configfs.c b/drivers/gpu/drm/vkms/vkms_configfs.c
new file mode 100644
index 0000000000000000000000000000000000000000..6535672f008401cf1ae008ff2ef74452b2575eab
--- /dev/null
+++ b/drivers/gpu/drm/vkms/vkms_configfs.c
@@ -0,0 +1,144 @@
+// SPDX-License-Identifier: GPL-2.0+
+
+#include <linux/configfs.h>
+#include <linux/mutex.h>
+#include <drm/drm_print.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+
+#include "vkms_configfs.h"
+#include "vkms_drv.h"
+#include "vkms_config.h"
+
+static ssize_t device_enable_show(struct config_item *item, char *page)
+{
+	return sprintf(page, "%d\n",
+		       config_item_to_vkms_configfs_device(item)->enabled);
+}
+
+static ssize_t device_enable_store(struct config_item *item,
+				   const char *page, size_t count)
+{
+	struct vkms_configfs_device *vkms_configfs_device =
+		config_item_to_vkms_configfs_device(item);
+
+	bool value;
+	int ret;
+
+	ret = kstrtobool(page, &value);
+	if (ret)
+		return -EINVAL;
+
+	mutex_lock(&vkms_configfs_device->lock);
+
+	vkms_configfs_device->enabled = value;
+
+	if (value)
+		vkms_create(vkms_configfs_device->vkms_config);
+	else
+		vkms_destroy(vkms_configfs_device->vkms_config);
+
+	mutex_unlock(&vkms_configfs_device->lock);
+
+	return (ssize_t)count;
+}
+
+CONFIGFS_ATTR(device_, enable);
+
+static struct configfs_attribute *device_attrs[] = {
+	&device_attr_enable,
+	NULL,
+};
+
+static void device_release(struct config_item *item)
+{
+	struct vkms_configfs_device *vkms_configfs_device =
+					    config_item_to_vkms_configfs_device(item);
+
+	mutex_destroy(&vkms_configfs_device->lock);
+	vkms_config_destroy(vkms_configfs_device->vkms_config);
+
+	kfree(vkms_configfs_device);
+}
+
+static struct configfs_item_operations device_item_operations = {
+	.release	= &device_release,
+};
+
+static const struct config_item_type device_item_type = {
+	.ct_attrs	= device_attrs,
+	.ct_item_ops	= &device_item_operations,
+	.ct_owner	= THIS_MODULE,
+};
+
+/* Top directory management. Each new directory here is a new device */
+static struct config_group *root_make_group(struct config_group *group,
+					    const char *name)
+{
+	struct vkms_config_plane *plane;
+	struct vkms_config_crtc *crtc;
+	struct vkms_config_encoder *encoder;
+	struct vkms_configfs_device *configfs = kzalloc(sizeof(*configfs), GFP_KERNEL);
+
+	if (!configfs)
+		return ERR_PTR(-ENOMEM);
+
+	mutex_init(&configfs->lock);
+
+	configfs->vkms_config = vkms_config_create();
+
+	if (!configfs->vkms_config) {
+		kfree(configfs);
+		return ERR_PTR(-ENOMEM);
+	}
+
+	plane = vkms_config_create_plane(configfs->vkms_config);
+	crtc = vkms_config_create_crtc(configfs->vkms_config);
+	encoder = vkms_config_create_encoder(configfs->vkms_config);
+
+	if (!plane || !crtc || !encoder ||
+	    vkms_config_plane_attach_crtc(plane, crtc) ||
+	    vkms_config_encoder_attach_crtc(encoder, crtc)) {
+		vkms_config_destroy(configfs->vkms_config);
+		kfree(configfs);
+		return ERR_PTR(-ENOMEM);
+	}
+
+	plane->type = DRM_PLANE_TYPE_PRIMARY;
+
+	config_group_init_type_name(&configfs->group, name,
+				    &device_item_type);
+
+	return &configfs->group;
+}
+
+static struct configfs_group_operations root_group_operations = {
+	.make_group	= &root_make_group,
+};
+
+static struct config_item_type root_item_type = {
+	.ct_group_ops	= &root_group_operations,
+	.ct_owner	= THIS_MODULE,
+};
+
+static struct configfs_subsystem vkms_subsys = {
+	.su_group = {
+		.cg_item = {
+			.ci_name = "vkms",
+			.ci_type = &root_item_type,
+		},
+	},
+	.su_mutex = __MUTEX_INITIALIZER(vkms_subsys.su_mutex),
+};
+
+int vkms_init_configfs(void)
+{
+	config_group_init(&vkms_subsys.su_group);
+
+	return configfs_register_subsystem(&vkms_subsys);
+}
+
+void vkms_unregister_configfs(void)
+{
+	configfs_unregister_subsystem(&vkms_subsys);
+}
diff --git a/drivers/gpu/drm/vkms/vkms_configfs.h b/drivers/gpu/drm/vkms/vkms_configfs.h
new file mode 100644
index 0000000000000000000000000000000000000000..3de89c9c552c6603396f38a98ad35263f04bce26
--- /dev/null
+++ b/drivers/gpu/drm/vkms/vkms_configfs.h
@@ -0,0 +1,35 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+
+#include <linux/configfs.h>
+#include <linux/mutex.h>
+
+#ifndef _VKMS_CONFIGFS_H
+#define _VKMS_CONFIGFS_H
+
+/**
+ * struct vkms_configfs_device - Internal object to manage all the configfs items related to one
+ * device
+ *
+ * @group: Main configfs group for a device
+ * @platform_device: If a device was created (@enabled = true), stores a pointer to it
+ * @lock: Mutex used to avoid conflicting edition of @vkms_config
+ * @enabled: Store if the device was created or not
+ * @vkms_config: Current vkms configuration
+ */
+struct vkms_configfs_device {
+	struct config_group group;
+
+	struct mutex lock;
+	bool enabled;
+
+	struct vkms_config *vkms_config;
+};
+
+#define config_item_to_vkms_configfs_device(item) \
+	container_of(to_config_group((item)), struct vkms_configfs_device, group)
+
+/* ConfigFS Support */
+int vkms_init_configfs(void);
+void vkms_unregister_configfs(void);
+
+#endif /* _VKMS_CONFIGFS_H */
diff --git a/drivers/gpu/drm/vkms/vkms_drv.c b/drivers/gpu/drm/vkms/vkms_drv.c
index 8ffbd5fc65350ba03f870726b669d189f62bad6f..cdf51df78802cd096b71cfc76678a9ebdd06df1a 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.c
+++ b/drivers/gpu/drm/vkms/vkms_drv.c
@@ -29,6 +29,7 @@
 
 #include "vkms_drv.h"
 #include "vkms_config.h"
+#include "vkms_configfs.h"
 
 #include <drm/drm_print.h>
 #include <drm/drm_debugfs.h>
@@ -158,7 +159,7 @@ int vkms_create(struct vkms_config *config)
 	struct platform_device *pdev;
 	struct vkms_device *vkms_device;
 
-	pdev = platform_device_register_simple(DRIVER_NAME, -1, NULL, 0);
+	pdev = platform_device_register_simple(DRIVER_NAME, PLATFORM_DEVID_AUTO, NULL, 0);
 	if (IS_ERR(pdev))
 		return PTR_ERR(pdev);
 
@@ -219,9 +220,17 @@ static int __init vkms_init(void)
 	if (IS_ERR(default_config))
 		return PTR_ERR(default_config);
 
+	ret = vkms_init_configfs();
+	if (ret) {
+		DRM_ERROR("Unable to initialize configfs\n");
+		vkms_config_destroy(default_config);
+	}
+
 	ret = vkms_create(default_config);
-	if (ret)
+	if (ret) {
+		vkms_unregister_configfs();
 		vkms_config_destroy(default_config);
+	}
 
 	return ret;
 }
@@ -251,6 +260,8 @@ static void __exit vkms_exit(void)
 		vkms_destroy(default_config);
 
 	vkms_config_destroy(default_config);
+
+	vkms_unregister_configfs();
 }
 
 module_init(vkms_init);

-- 
2.47.0

[PATCH RFC v2 04/16] drm/vkms: Introduce configfs for plane

[Louis Chauvet]

To allows the userspace to test many hardware configuration, introduce a
new interface to create and configure planes.

The planes are created by creating a directory in the `planes` directory.
The type of plane is configured by writing 0 (Overlay), 1 (primary) or 2
(cursor) in the file `type`.

As the CRTCs and encoders can't be configured yet, the planes are all
atteched to the same CRTC and encoder.

The current interface is:
/config/vkms
       	DEVICE_1
       	┣━ enable
       	┣━ planes
       	┃  ┣━ PLANE_1
       	┃  ┃  ┣━ type
       	┃  ┣━ PLANE_2
       	┃  ┃  ┗━ type
       	┃  ┗━ PLANE_3
       	┃     ┗━ type
       	DEVICE_2
       	┗━ ditto

Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
---
 drivers/gpu/drm/vkms/vkms_configfs.c | 149 ++++++++++++++++++++++++++++++++---
 drivers/gpu/drm/vkms/vkms_configfs.h |  20 +++++
 2 files changed, 160 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/vkms/vkms_configfs.c b/drivers/gpu/drm/vkms/vkms_configfs.c
index 6535672f008401cf1ae008ff2ef74452b2575eab..ee64243396b3e586f76ff0671441c172da022421 100644
--- a/drivers/gpu/drm/vkms/vkms_configfs.c
+++ b/drivers/gpu/drm/vkms/vkms_configfs.c
@@ -10,6 +10,138 @@
 #include "vkms_drv.h"
 #include "vkms_config.h"
 
+static ssize_t plane_type_show(struct config_item *item, char *page)
+{
+	struct vkms_config_plane *plane;
+	enum drm_plane_type plane_type;
+	struct vkms_configfs_device *vkms_configfs = plane_item_to_vkms_configfs_device(item);
+
+	mutex_lock(&vkms_configfs->lock);
+	plane = plane_item_to_vkms_configfs_plane(item)->vkms_config_plane;
+	plane_type = plane->type;
+	mutex_unlock(&vkms_configfs->lock);
+
+	return sprintf(page, "%u", plane_type);
+}
+
+static ssize_t plane_type_store(struct config_item *item,
+				const char *page, size_t count)
+{
+	struct vkms_configfs_device *vkms_configfs = plane_item_to_vkms_configfs_device(item);
+	enum drm_plane_type val = DRM_PLANE_TYPE_OVERLAY;
+	struct vkms_config_plane *plane;
+	int ret;
+
+	ret = kstrtouint(page, 10, &val);
+	if (ret)
+		return ret;
+
+	if (val != DRM_PLANE_TYPE_PRIMARY && val != DRM_PLANE_TYPE_CURSOR &&
+	    val != DRM_PLANE_TYPE_OVERLAY)
+		return -EINVAL;
+
+	mutex_lock(&vkms_configfs->lock);
+	if (vkms_configfs->enabled) {
+		mutex_unlock(&vkms_configfs->lock);
+		return -EINVAL;
+	}
+
+	plane = plane_item_to_vkms_configfs_plane(item)->vkms_config_plane;
+	plane->type = val;
+
+	mutex_unlock(&vkms_configfs->lock);
+
+	return count;
+}
+
+CONFIGFS_ATTR(plane_, type);
+
+static struct configfs_attribute *plane_attrs[] = {
+	&plane_attr_type,
+	NULL,
+};
+
+static void plane_release(struct config_item *item)
+{
+	struct vkms_configfs_plane *vkms_configfs_plane = plane_item_to_vkms_configfs_plane(item);
+
+	mutex_lock(&vkms_configfs_plane->vkms_configfs_device->lock);
+	vkms_config_delete_plane(vkms_configfs_plane->vkms_config_plane,
+				 vkms_configfs_plane->vkms_configfs_device->vkms_config);
+	mutex_unlock(&vkms_configfs_plane->vkms_configfs_device->lock);
+
+	kfree(vkms_configfs_plane);
+}
+
+static struct configfs_item_operations plane_item_operations = {
+	.release	= plane_release,
+};
+
+static const struct config_item_type subgroup_plane = {
+	.ct_attrs	= plane_attrs,
+	.ct_item_ops	= &plane_item_operations,
+	.ct_owner	= THIS_MODULE,
+};
+
+static struct config_group *planes_make_group(struct config_group *config_group,
+					      const char *name)
+{
+	struct vkms_configfs_device *vkms_configfs;
+	struct vkms_configfs_plane *vkms_configfs_plane;
+
+	vkms_configfs = planes_item_to_vkms_configfs_device(&config_group->cg_item);
+	vkms_configfs_plane = kzalloc(sizeof(*vkms_configfs_plane), GFP_KERNEL);
+
+	if (!vkms_configfs_plane)
+		return ERR_PTR(-ENOMEM);
+
+	mutex_lock(&vkms_configfs->lock);
+
+	if (vkms_configfs->enabled) {
+		kfree(vkms_configfs_plane);
+		mutex_unlock(&vkms_configfs->lock);
+		return ERR_PTR(-EINVAL);
+	}
+
+	vkms_configfs_plane->vkms_config_plane = vkms_config_create_plane(vkms_configfs->vkms_config);
+
+	if (list_count_nodes(&vkms_configfs->vkms_config->planes) == 1)
+		vkms_configfs_plane->vkms_config_plane->type = DRM_PLANE_TYPE_PRIMARY;
+
+	if (!vkms_configfs_plane->vkms_config_plane ||
+	    vkms_config_plane_attach_crtc(vkms_configfs_plane->vkms_config_plane,
+					  vkms_configfs->vkms_config_crtc)) {
+		kfree(vkms_configfs_plane);
+		mutex_unlock(&vkms_configfs->lock);
+		return ERR_PTR(-ENOMEM);
+	}
+
+	vkms_configfs_plane->vkms_config_plane->name = kzalloc(strlen(name) + 1, GFP_KERNEL);
+	if (!vkms_configfs_plane->vkms_config_plane->name) {
+		kfree(vkms_configfs_plane->vkms_config_plane);
+		kfree(vkms_configfs_plane);
+		mutex_unlock(&vkms_configfs->lock);
+		return ERR_PTR(-ENOMEM);
+	}
+	strscpy(vkms_configfs_plane->vkms_config_plane->name, name, strlen(name) + 1);
+
+	config_group_init_type_name(&vkms_configfs_plane->group, name, &subgroup_plane);
+
+	vkms_configfs_plane->vkms_configfs_device = vkms_configfs;
+	mutex_unlock(&vkms_configfs->lock);
+
+	return &vkms_configfs_plane->group;
+}
+
+static struct configfs_group_operations planes_group_operations = {
+	.make_group	= &planes_make_group,
+};
+
+static const struct config_item_type planes_item_type = {
+	.ct_group_ops	= &planes_group_operations,
+	.ct_owner	= THIS_MODULE,
+};
+
 static ssize_t device_enable_show(struct config_item *item, char *page)
 {
 	return sprintf(page, "%d\n",
@@ -92,23 +224,22 @@ static struct config_group *root_make_group(struct config_group *group,
 		return ERR_PTR(-ENOMEM);
 	}
 
-	plane = vkms_config_create_plane(configfs->vkms_config);
-	crtc = vkms_config_create_crtc(configfs->vkms_config);
-	encoder = vkms_config_create_encoder(configfs->vkms_config);
-
-	if (!plane || !crtc || !encoder ||
-	    vkms_config_plane_attach_crtc(plane, crtc) ||
-	    vkms_config_encoder_attach_crtc(encoder, crtc)) {
+	configfs->vkms_config_crtc = vkms_config_create_crtc(configfs->vkms_config);
+	configfs->vkms_config_encoder = vkms_config_create_encoder(configfs->vkms_config);
+	if (!configfs->vkms_config_crtc || !configfs->vkms_config_encoder ||
+	    vkms_config_encoder_attach_crtc(configfs->vkms_config_encoder,
+					    configfs->vkms_config_crtc)) {
 		vkms_config_destroy(configfs->vkms_config);
 		kfree(configfs);
 		return ERR_PTR(-ENOMEM);
 	}
 
-	plane->type = DRM_PLANE_TYPE_PRIMARY;
-
 	config_group_init_type_name(&configfs->group, name,
 				    &device_item_type);
 
+	config_group_init_type_name(&configfs->plane_group, "planes", &planes_item_type);
+	configfs_add_default_group(&configfs->plane_group, &configfs->group);
+
 	return &configfs->group;
 }
 
diff --git a/drivers/gpu/drm/vkms/vkms_configfs.h b/drivers/gpu/drm/vkms/vkms_configfs.h
index 3de89c9c552c6603396f38a98ad35263f04bce26..6dc4d34a9e44ed4beb115ad3c86b759e28f5d0ef 100644
--- a/drivers/gpu/drm/vkms/vkms_configfs.h
+++ b/drivers/gpu/drm/vkms/vkms_configfs.h
@@ -19,15 +19,35 @@
 struct vkms_configfs_device {
 	struct config_group group;
 
+	struct config_group plane_group;
+
 	struct mutex lock;
 	bool enabled;
 
 	struct vkms_config *vkms_config;
+	struct vkms_config_crtc *vkms_config_crtc;
+	struct vkms_config_encoder *vkms_config_encoder;
+};
+
+struct vkms_configfs_plane {
+	struct config_group group;
+
+	struct vkms_configfs_device *vkms_configfs_device;
+	struct vkms_config_plane *vkms_config_plane;
 };
 
 #define config_item_to_vkms_configfs_device(item) \
 	container_of(to_config_group((item)), struct vkms_configfs_device, group)
 
+#define planes_item_to_vkms_configfs_device(item) \
+	config_item_to_vkms_configfs_device((item)->ci_parent)
+
+#define plane_item_to_vkms_configfs_device(item) \
+	planes_item_to_vkms_configfs_device((item)->ci_parent)
+
+#define plane_item_to_vkms_configfs_plane(item) \
+	container_of(to_config_group((item)), struct vkms_configfs_plane, group)
+
 /* ConfigFS Support */
 int vkms_init_configfs(void);
 void vkms_unregister_configfs(void);

-- 
2.47.0

[PATCH RFC v2 05/16] drm/vkms: Introduce configfs for plane rotation

[Louis Chauvet]

To allows the userspace to test many hardware configuration, introduce a
new interface to configure the available rotation per planes. VKMS
supports any rotation and reflection, so the userspace can choose any
combination.

The supported rotations are configured by writing a rotation bitmask to
the file `supported_rotations` and the default rotation is chosen by
writing a rotation bitmask to `default_rotation`.

The current interface is:
/config/vkms
       	DEVICE_1
       	┣━ enable
       	┣━ planes
       	┃  ┗━ PLANE_1
       	┃     ┣━ type
       	┃     ┣━ supported_rotations
       	┃     ┗━ default_rotation
       	DEVICE_2
       	┗━ ditto

Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
---
 drivers/gpu/drm/vkms/vkms_configfs.c | 102 +++++++++++++++++++++++++++++++++++
 1 file changed, 102 insertions(+)

diff --git a/drivers/gpu/drm/vkms/vkms_configfs.c b/drivers/gpu/drm/vkms/vkms_configfs.c
index ee64243396b3e586f76ff0671441c172da022421..d121cf54e75238bdb582362596a353682cceebd3 100644
--- a/drivers/gpu/drm/vkms/vkms_configfs.c
+++ b/drivers/gpu/drm/vkms/vkms_configfs.c
@@ -54,10 +54,112 @@ static ssize_t plane_type_store(struct config_item *item,
 	return count;
 }
 
+static ssize_t plane_supported_rotations_show(struct config_item *item, char *page)
+{
+	struct vkms_config_plane *plane;
+	unsigned int plane_type;
+	struct vkms_configfs_device *vkms_configfs = plane_item_to_vkms_configfs_device(item);
+
+	mutex_lock(&vkms_configfs->lock);
+	plane = plane_item_to_vkms_configfs_plane(item)->vkms_config_plane;
+	plane_type = plane->supported_rotations;
+	mutex_unlock(&vkms_configfs->lock);
+
+	return sprintf(page, "%u", plane_type);
+}
+
+static ssize_t plane_supported_rotations_store(struct config_item *item,
+					       const char *page, size_t count)
+{
+	struct vkms_config_plane *plane;
+	struct vkms_configfs_device *vkms_configfs = plane_item_to_vkms_configfs_device(item);
+	int ret, val = 0;
+
+	ret = kstrtouint(page, 0, &val);
+	if (ret)
+		return ret;
+
+	/* Should be a supported value */
+	if (val & ~(DRM_MODE_ROTATE_MASK | DRM_MODE_REFLECT_MASK))
+		return -EINVAL;
+	/* Should at least provide one rotation */
+	if (!(val & DRM_MODE_ROTATE_MASK))
+		return -EINVAL;
+
+	mutex_lock(&vkms_configfs->lock);
+
+	plane = plane_item_to_vkms_configfs_plane(item)->vkms_config_plane;
+
+	/* Ensures that the default rotation is included in supported rotation */
+	if (vkms_configfs->enabled || (val & plane->default_rotation) != plane->default_rotation) {
+		mutex_unlock(&vkms_configfs->lock);
+		return -EINVAL;
+	}
+
+	plane->supported_rotations = val;
+	mutex_unlock(&vkms_configfs->lock);
+
+	return count;
+}
+
+static ssize_t plane_default_rotation_show(struct config_item *item, char *page)
+{
+	unsigned int plane_type;
+	struct vkms_config_plane *plane;
+	struct vkms_configfs_device *vkms_configfs = plane_item_to_vkms_configfs_device(item);
+
+	mutex_lock(&vkms_configfs->lock);
+	plane = plane_item_to_vkms_configfs_plane(item)->vkms_config_plane;
+	plane_type = plane->default_rotation;
+	mutex_unlock(&vkms_configfs->lock);
+
+	return sprintf(page, "%u", plane_type);
+}
+
+static ssize_t plane_default_rotation_store(struct config_item *item,
+					    const char *page, size_t count)
+{
+	struct vkms_config_plane *plane;
+	struct vkms_configfs_device *vkms_configfs = plane_item_to_vkms_configfs_device(item);
+	int ret, val = 0;
+
+	ret = kstrtouint(page, 10, &val);
+	if (ret)
+		return ret;
+
+	/* Should be a supported value */
+	if (val & ~(DRM_MODE_ROTATE_MASK | DRM_MODE_REFLECT_MASK))
+		return -EINVAL;
+	/* Should at least provide one rotation */
+	if ((val & DRM_MODE_ROTATE_MASK) == 0)
+		return -EINVAL;
+	/* Should contains only one rotation */
+	if (!is_power_of_2(val & DRM_MODE_ROTATE_MASK))
+		return -EINVAL;
+	mutex_lock(&vkms_configfs->lock);
+
+	plane = plane_item_to_vkms_configfs_plane(item)->vkms_config_plane;
+
+	/* Ensures that the default rotation is included in supported rotation */
+	if (vkms_configfs->enabled ||
+	    (val & plane->supported_rotations) != val) {
+		mutex_unlock(&vkms_configfs->lock);
+		return -EINVAL;
+	}
+	plane->default_rotation = val;
+	mutex_unlock(&vkms_configfs->lock);
+
+	return count;
+}
+
 CONFIGFS_ATTR(plane_, type);
+CONFIGFS_ATTR(plane_, supported_rotations);
+CONFIGFS_ATTR(plane_, default_rotation);
 
 static struct configfs_attribute *plane_attrs[] = {
 	&plane_attr_type,
+	&plane_attr_supported_rotations,
+	&plane_attr_default_rotation,
 	NULL,
 };
 

-- 
2.47.0

[PATCH RFC v2 06/16] drm/vkms: Introduce configfs for plane color encoding

[Louis Chauvet]

To allows the userspace to test many hardware configuration, introduce a
new interface to configure the available color encoding per planes. VKMS
supports multiple color encoding, so the userspace can choose any
combination.

The supported color encoding are configured by writing a color encoding
bitmask to the file `supported_color_encoding` and the default color
encoding is chosen by writing a color encoding bitmask to
`default_color_encoding`.

The current interface is:
/config/vkms
       	DEVICE_1
       	┣━ enable
       	┣━ planes
       	┃  ┗━ PLANE_1
       	┃     ┣━ type
       	┃     ┣━ supported_rotations
       	┃     ┣━ supported_color_encoding
       	┃     ┣━ default_rotation
       	┃     ┗━ default_color_encoding
       	DEVICE_2
       	┗━ ditto

Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
---
 drivers/gpu/drm/vkms/vkms_configfs.c | 107 +++++++++++++++++++++++++++++++++++
 1 file changed, 107 insertions(+)

diff --git a/drivers/gpu/drm/vkms/vkms_configfs.c b/drivers/gpu/drm/vkms/vkms_configfs.c
index d121cf54e75238bdb582362596a353682cceebd3..a3dab59868829266adfe8192c5089cda2044c050 100644
--- a/drivers/gpu/drm/vkms/vkms_configfs.c
+++ b/drivers/gpu/drm/vkms/vkms_configfs.c
@@ -152,14 +152,121 @@ static ssize_t plane_default_rotation_store(struct config_item *item,
 	return count;
 }
 
+static ssize_t plane_supported_color_encoding_show(struct config_item *item, char *page)
+{
+	struct vkms_config_plane *plane;
+	unsigned int supported_color_encoding;
+	struct vkms_configfs_device *vkms_configfs = plane_item_to_vkms_configfs_device(item);
+
+	mutex_lock(&vkms_configfs->lock);
+	plane = plane_item_to_vkms_configfs_plane(item)->vkms_config_plane;
+	supported_color_encoding = plane->supported_color_encoding;
+	mutex_unlock(&vkms_configfs->lock);
+
+	return sprintf(page, "%u", supported_color_encoding);
+}
+
+static ssize_t plane_supported_color_encoding_store(struct config_item *item,
+						    const char *page, size_t count)
+{
+	struct vkms_configfs_device *vkms_configfs = plane_item_to_vkms_configfs_device(item);
+	struct vkms_config_plane *plane;
+	int ret, val = 0;
+
+	ret = kstrtouint(page, 10, &val);
+	if (ret)
+		return ret;
+
+	/* Should be a supported value */
+	if (val & ~(BIT(DRM_COLOR_YCBCR_BT601) |
+		    BIT(DRM_COLOR_YCBCR_BT709) |
+		    BIT(DRM_COLOR_YCBCR_BT2020)))
+		return -EINVAL;
+	/* Should at least provide one color range */
+	if ((val & (BIT(DRM_COLOR_YCBCR_BT601) |
+		    BIT(DRM_COLOR_YCBCR_BT709) |
+		    BIT(DRM_COLOR_YCBCR_BT2020))) == 0)
+		return -EINVAL;
+
+	mutex_lock(&vkms_configfs->lock);
+	plane = plane_item_to_vkms_configfs_plane(item)->vkms_config_plane;
+
+	/* Ensures that the default rotation is included in supported rotation */
+	if (vkms_configfs->enabled || (val & plane->default_color_encoding) !=
+				      plane->default_color_encoding) {
+		mutex_unlock(&vkms_configfs->lock);
+		return -EINVAL;
+	}
+	plane->supported_color_encoding = val;
+	mutex_unlock(&vkms_configfs->lock);
+
+	return count;
+}
+
+/* Plane default_color_encoding : vkms/<device>/planes/<plane>/default_color_encoding */
+
+static ssize_t plane_default_color_encoding_show(struct config_item *item, char *page)
+{
+	struct vkms_config_plane *plane;
+	unsigned int default_color_encoding;
+	struct vkms_configfs_device *vkms_configfs = plane_item_to_vkms_configfs_device(item);
+
+	mutex_lock(&vkms_configfs->lock);
+	plane = plane_item_to_vkms_configfs_plane(item)->vkms_config_plane;
+	default_color_encoding = plane->default_color_encoding;
+	mutex_unlock(&vkms_configfs->lock);
+
+	return sprintf(page, "%u", default_color_encoding);
+}
+
+static ssize_t plane_default_color_encoding_store(struct config_item *item,
+						  const char *page, size_t count)
+{
+	struct vkms_config_plane *plane;
+	struct vkms_configfs_device *vkms_configfs = plane_item_to_vkms_configfs_device(item);
+	int ret, val = 0;
+
+	ret = kstrtouint(page, 10, &val);
+	if (ret)
+		return ret;
+
+	/* Should be a supported value */
+	if (val & ~(BIT(DRM_COLOR_YCBCR_BT601) |
+		    BIT(DRM_COLOR_YCBCR_BT709) |
+		    BIT(DRM_COLOR_YCBCR_BT2020)))
+		return -EINVAL;
+	/* Should at least provide one color range */
+	if ((val & (BIT(DRM_COLOR_YCBCR_BT601) |
+		    BIT(DRM_COLOR_YCBCR_BT709) |
+		    BIT(DRM_COLOR_YCBCR_BT2020))) == 0)
+		return -EINVAL;
+	mutex_lock(&vkms_configfs->lock);
+
+	plane = plane_item_to_vkms_configfs_plane(item)->vkms_config_plane;
+
+	/* Ensures that the default rotation is included in supported rotation */
+	if (vkms_configfs->enabled || (val & plane->supported_color_encoding) != val) {
+		mutex_unlock(&vkms_configfs->lock);
+		return -EINVAL;
+	}
+	plane->default_color_encoding = val;
+	mutex_unlock(&vkms_configfs->lock);
+
+	return count;
+}
+
 CONFIGFS_ATTR(plane_, type);
 CONFIGFS_ATTR(plane_, supported_rotations);
 CONFIGFS_ATTR(plane_, default_rotation);
+CONFIGFS_ATTR(plane_, supported_color_encoding);
+CONFIGFS_ATTR(plane_, default_color_encoding);
 
 static struct configfs_attribute *plane_attrs[] = {
 	&plane_attr_type,
 	&plane_attr_supported_rotations,
 	&plane_attr_default_rotation,
+	&plane_attr_supported_color_encoding,
+	&plane_attr_default_color_encoding,
 	NULL,
 };
 

-- 
2.47.0

[PATCH RFC v2 07/16] drm/vkms: Introduce configfs for plane color range

[Louis Chauvet]

To allows the userspace to test many hardware configuration, introduce a
new interface to configure the available color ranges per planes. VKMS
supports multiple color ranges, so the userspace can choose any
combination.

The supported color ranges are configured by writing a color range bitmask
to the file `supported_color_ranges` and the default color range is
chosen by writing a color encoding bitmask to `default_color_range`.

The current interface is:
/config/vkms
       	DEVICE_1
       	┣━ enable
       	┣━ planes
       	┃  ┗━ PLANE_1
       	┃     ┣━ type
       	┃     ┣━ supported_rotations
       	┃     ┣━ supported_color_encoding
       	┃     ┣━ supported_color_ranges
       	┃     ┣━ default_rotation
       	┃     ┣━ default_color_encoding
       	┃     ┗━ default_color_range
       	DEVICE_2
       	┗━ ditto

Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
---
 drivers/gpu/drm/vkms/vkms_configfs.c | 103 +++++++++++++++++++++++++++++++++++
 1 file changed, 103 insertions(+)

diff --git a/drivers/gpu/drm/vkms/vkms_configfs.c b/drivers/gpu/drm/vkms/vkms_configfs.c
index a3dab59868829266adfe8192c5089cda2044c050..aabc832836266668c8adc60d7d5284ee1f385f31 100644
--- a/drivers/gpu/drm/vkms/vkms_configfs.c
+++ b/drivers/gpu/drm/vkms/vkms_configfs.c
@@ -152,6 +152,105 @@ static ssize_t plane_default_rotation_store(struct config_item *item,
 	return count;
 }
 
+static ssize_t plane_color_range_show(struct config_item *item, char *page)
+{
+	struct vkms_config_plane *plane;
+	unsigned int plane_type;
+	struct vkms_configfs_device *vkms_configfs = plane_item_to_vkms_configfs_device(item);
+
+	mutex_lock(&vkms_configfs->lock);
+	plane = plane_item_to_vkms_configfs_plane(item)->vkms_config_plane;
+	plane_type = plane->supported_color_range;
+	mutex_unlock(&vkms_configfs->lock);
+
+	return sprintf(page, "%u", plane_type);
+}
+
+static ssize_t plane_color_range_store(struct config_item *item,
+				       const char *page, size_t count)
+{
+	struct vkms_config_plane *plane;
+	struct vkms_configfs_device *vkms_configfs = plane_item_to_vkms_configfs_device(item);
+	int ret, val = 0;
+
+	ret = kstrtouint(page, 10, &val);
+	if (ret)
+		return ret;
+
+	/* Should be a supported value */
+	if (val & ~(BIT(DRM_COLOR_YCBCR_LIMITED_RANGE) |
+		    BIT(DRM_COLOR_YCBCR_FULL_RANGE)))
+		return -EINVAL;
+	/* Should at least provide one color range */
+	if ((val & (BIT(DRM_COLOR_YCBCR_LIMITED_RANGE) |
+		    BIT(DRM_COLOR_YCBCR_FULL_RANGE))) == 0)
+		return -EINVAL;
+
+	mutex_lock(&vkms_configfs->lock);
+
+	plane = plane_item_to_vkms_configfs_plane(item)->vkms_config_plane;
+
+	/* Ensures that the default rotation is included in supported rotation */
+	if (vkms_configfs->enabled || (val & plane->default_color_range) !=
+				      plane->default_color_range) {
+		mutex_unlock(&vkms_configfs->lock);
+		return -EINVAL;
+	}
+	plane->supported_color_range = val;
+	mutex_unlock(&vkms_configfs->lock);
+
+	return count;
+}
+
+static ssize_t plane_default_color_range_show(struct config_item *item, char *page)
+{
+	struct vkms_config_plane *plane;
+	unsigned int plane_type;
+	struct vkms_configfs_device *vkms_configfs = plane_item_to_vkms_configfs_device(item);
+
+	mutex_lock(&vkms_configfs->lock);
+	plane = plane_item_to_vkms_configfs_plane(item)->vkms_config_plane;
+	plane_type = plane->default_color_range;
+	mutex_unlock(&vkms_configfs->lock);
+
+	return sprintf(page, "%u", plane_type);
+}
+
+static ssize_t plane_default_color_range_store(struct config_item *item,
+					       const char *page, size_t count)
+{
+	struct vkms_config_plane *plane;
+	struct vkms_configfs_device *vkms_configfs = plane_item_to_vkms_configfs_device(item);
+	int ret, val = 0;
+
+	ret = kstrtouint(page, 10, &val);
+	if (ret)
+		return ret;
+
+	/* Should be a supported value */
+	if (val & ~(BIT(DRM_COLOR_YCBCR_LIMITED_RANGE) |
+		    BIT(DRM_COLOR_YCBCR_FULL_RANGE)))
+		return -EINVAL;
+	/* Should at least provide one color range */
+	if ((val & (BIT(DRM_COLOR_YCBCR_LIMITED_RANGE) |
+		    BIT(DRM_COLOR_YCBCR_FULL_RANGE))) == 0)
+		return -EINVAL;
+
+	mutex_lock(&vkms_configfs->lock);
+
+	plane = plane_item_to_vkms_configfs_plane(item)->vkms_config_plane;
+
+	/* Ensures that the default rotation is included in supported rotation */
+	if (vkms_configfs->enabled || (val & plane->supported_color_range) != val) {
+		mutex_unlock(&vkms_configfs->lock);
+		return -EINVAL;
+	}
+	plane->default_color_range = val;
+	mutex_unlock(&vkms_configfs->lock);
+
+	return count;
+}
+
 static ssize_t plane_supported_color_encoding_show(struct config_item *item, char *page)
 {
 	struct vkms_config_plane *plane;
@@ -258,6 +357,8 @@ static ssize_t plane_default_color_encoding_store(struct config_item *item,
 CONFIGFS_ATTR(plane_, type);
 CONFIGFS_ATTR(plane_, supported_rotations);
 CONFIGFS_ATTR(plane_, default_rotation);
+CONFIGFS_ATTR(plane_, color_range);
+CONFIGFS_ATTR(plane_, default_color_range);
 CONFIGFS_ATTR(plane_, supported_color_encoding);
 CONFIGFS_ATTR(plane_, default_color_encoding);
 
@@ -265,6 +366,8 @@ static struct configfs_attribute *plane_attrs[] = {
 	&plane_attr_type,
 	&plane_attr_supported_rotations,
 	&plane_attr_default_rotation,
+	&plane_attr_color_range,
+	&plane_attr_default_color_range,
 	&plane_attr_supported_color_encoding,
 	&plane_attr_default_color_encoding,
 	NULL,

-- 
2.47.0

[PATCH RFC v2 08/16] drm/vkms: Introduce configfs for crtc and encoder

[Louis Chauvet]

To allows the userspace to test many hardware configuration, introduce a
new interface to configure CRTCs and encoders.

The CRTCs and encoders are created in their own directory. To link the
CRTC, symlinks are used in the `possible_crtcs` folders.

The current interface is:
/config/vkms
	DEVICE_1
	┣━ enable
	┣━ planes
	┃  ┗━ PLANE_1
	┃     ┣━ type
	┃     ┣━ supported_rotations
	┃     ┣━ supported_color_encoding
	┃     ┣━ supported_color_ranges
	┃     ┣━ default_rotation
	┃     ┣━ default_color_encoding
	┃     ┣━ default_color_range
	┃     ┗━ possible_crtcs
	┃        ┗━ >> /config/vkms/DEVICE_1/crtcs/CRTC_1
	┣━ encoders
	┃  ┗━ ENCODER_1
	┃     ┗━ possible_crtcs
	┃        ┗━ >> /config/vkms/DEVICE_1/crtcs/CRTC_1
	┣━ crtcs
	┃  ┗━ CRTC_1
	DEVICE_2
	┗━ ditto

Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
---
 drivers/gpu/drm/vkms/vkms_configfs.c | 401 +++++++++++++++++++++++++++++++++--
 drivers/gpu/drm/vkms/vkms_configfs.h |  54 ++++-
 2 files changed, 434 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/vkms/vkms_configfs.c b/drivers/gpu/drm/vkms/vkms_configfs.c
index aabc832836266668c8adc60d7d5284ee1f385f31..a410c9be4f2bbf7b2651245747eb357fcf32d1f2 100644
--- a/drivers/gpu/drm/vkms/vkms_configfs.c
+++ b/drivers/gpu/drm/vkms/vkms_configfs.c
@@ -5,6 +5,7 @@
 #include <drm/drm_print.h>
 #include <linux/platform_device.h>
 #include <linux/slab.h>
+#include <linux/generic-radix-tree.h>
 
 #include "vkms_configfs.h"
 #include "vkms_drv.h"
@@ -395,6 +396,84 @@ static const struct config_item_type subgroup_plane = {
 	.ct_owner	= THIS_MODULE,
 };
 
+static const struct config_item_type crtc_item_type;
+static const struct config_item_type planes_item_type;
+
+static int possible_crtcs_allow_link(struct config_item *src,
+				     struct config_item *target)
+{
+	struct vkms_configfs_device *vkms_configfs = plane_possible_crtc_src_item_to_vkms_configfs_device(src);
+	struct vkms_config_crtc *crtc;
+
+	mutex_lock(&vkms_configfs->lock);
+
+	if (target->ci_type != &crtc_item_type) {
+		mutex_unlock(&vkms_configfs->lock);
+		return -EINVAL;
+	}
+
+	crtc = crtc_item_to_vkms_configfs_crtc(target)->vkms_config_crtc;
+	struct vkms_config_plane *plane = plane_possible_crtc_src_item_to_vkms_configfs_plane(src)->vkms_config_plane;
+
+	struct vkms_config_crtc *crtc_entry;
+	unsigned long idx = 0;
+
+	xa_for_each(&plane->possible_crtcs, idx, crtc_entry) {
+		if (crtc_entry == crtc) {
+			mutex_unlock(&vkms_configfs->lock);
+			return -EINVAL;
+		}
+	}
+
+	if (vkms_config_plane_attach_crtc(plane, crtc))
+		return -EINVAL;
+
+	mutex_unlock(&vkms_configfs->lock);
+
+	return 0;
+}
+
+static void possible_crtcs_drop_link(struct config_item *src,
+				     struct config_item *target)
+{
+	struct vkms_config_crtc *crtc;
+	struct vkms_configfs_device *vkms_configfs = plane_possible_crtc_src_item_to_vkms_configfs_device(src);
+
+	mutex_lock(&vkms_configfs->lock);
+
+	crtc = crtc_item_to_vkms_configfs_crtc(target)->vkms_config_crtc;
+	struct vkms_config_plane *plane = plane_possible_crtc_src_item_to_vkms_configfs_plane(src)->vkms_config_plane;
+
+	struct vkms_config_crtc  *crtc_entry;
+	struct vkms_config_plane *plane_entry;
+	unsigned long crtc_idx  = -1;
+
+	xa_for_each(&plane->possible_crtcs, crtc_idx, crtc_entry) {
+		if (crtc_entry == crtc)
+			break;
+	}
+	unsigned long plane_idx = -1;
+
+	xa_erase(&plane->possible_crtcs, crtc_idx);
+	xa_for_each(&crtc->possible_planes, plane_idx, plane_entry) {
+		if (plane_entry == plane)
+			break;
+	}
+	xa_erase(&crtc->possible_planes, plane_idx);
+
+	mutex_unlock(&vkms_configfs->lock);
+}
+
+static struct configfs_item_operations plane_possible_crtcs_item_ops = {
+	.allow_link = &possible_crtcs_allow_link,
+	.drop_link = &possible_crtcs_drop_link,
+};
+
+static struct config_item_type plane_possible_crtcs_group_type = {
+	.ct_item_ops = &plane_possible_crtcs_item_ops,
+	.ct_owner = THIS_MODULE,
+};
+
 static struct config_group *planes_make_group(struct config_group *config_group,
 					      const char *name)
 {
@@ -419,10 +498,7 @@ static struct config_group *planes_make_group(struct config_group *config_group,
 
 	if (list_count_nodes(&vkms_configfs->vkms_config->planes) == 1)
 		vkms_configfs_plane->vkms_config_plane->type = DRM_PLANE_TYPE_PRIMARY;
-
-	if (!vkms_configfs_plane->vkms_config_plane ||
-	    vkms_config_plane_attach_crtc(vkms_configfs_plane->vkms_config_plane,
-					  vkms_configfs->vkms_config_crtc)) {
+	if (!vkms_configfs_plane->vkms_config_plane) {
 		kfree(vkms_configfs_plane);
 		mutex_unlock(&vkms_configfs->lock);
 		return ERR_PTR(-ENOMEM);
@@ -439,7 +515,12 @@ static struct config_group *planes_make_group(struct config_group *config_group,
 
 	config_group_init_type_name(&vkms_configfs_plane->group, name, &subgroup_plane);
 
+	config_group_init_type_name(&vkms_configfs_plane->possible_crtc_group, "possible_crtcs",
+				    &plane_possible_crtcs_group_type);
+	configfs_add_default_group(&vkms_configfs_plane->possible_crtc_group,
+				   &vkms_configfs_plane->group);
 	vkms_configfs_plane->vkms_configfs_device = vkms_configfs;
+
 	mutex_unlock(&vkms_configfs->lock);
 
 	return &vkms_configfs_plane->group;
@@ -454,6 +535,283 @@ static const struct config_item_type planes_item_type = {
 	.ct_owner	= THIS_MODULE,
 };
 
+static void crtc_release(struct config_item *item)
+{
+	struct vkms_configfs_crtc *vkms_configfs_crtc = crtc_item_to_vkms_configfs_crtc(item);
+
+	mutex_lock(&vkms_configfs_crtc->vkms_configfs_device->lock);
+	vkms_config_delete_crtc(vkms_configfs_crtc->vkms_config_crtc,
+				vkms_configfs_crtc->vkms_configfs_device->vkms_config);
+	mutex_unlock(&vkms_configfs_crtc->vkms_configfs_device->lock);
+
+	kfree(vkms_configfs_crtc);
+}
+
+static struct configfs_item_operations crtc_item_operations = {
+	.release = crtc_release,
+};
+
+static const struct config_item_type crtc_item_type = {
+	.ct_owner	= THIS_MODULE,
+	.ct_item_ops	= &crtc_item_operations,
+};
+
+static struct config_group *crtcs_make_group(struct config_group *config_group,
+					     const char *name)
+{
+	struct config_item *root_item = config_group->cg_item.ci_parent;
+	struct vkms_configfs_device *vkms_configfs = config_item_to_vkms_configfs_device(root_item);
+	struct vkms_configfs_crtc *vkms_configfs_crtc;
+
+	vkms_configfs_crtc = kzalloc(sizeof(*vkms_configfs_crtc), GFP_KERNEL);
+
+	if (!vkms_configfs_crtc)
+		return ERR_PTR(-ENOMEM);
+
+	mutex_lock(&vkms_configfs->lock);
+	vkms_configfs_crtc->vkms_configfs_device = vkms_configfs;
+
+	if (vkms_configfs->enabled) {
+		kfree(vkms_configfs_crtc);
+		mutex_unlock(&vkms_configfs->lock);
+		return ERR_PTR(-EINVAL);
+	}
+
+	vkms_configfs_crtc->vkms_config_crtc = vkms_config_create_crtc(vkms_configfs->vkms_config);
+
+	if (!vkms_configfs_crtc->vkms_config_crtc) {
+		kfree(vkms_configfs_crtc);
+		mutex_unlock(&vkms_configfs->lock);
+		return ERR_PTR(-ENOMEM);
+	}
+
+	vkms_configfs_crtc->vkms_config_crtc->name = kzalloc(strlen(name) + 1, GFP_KERNEL);
+	if (!vkms_configfs_crtc->vkms_config_crtc->name) {
+		kfree(vkms_configfs_crtc->vkms_config_crtc);
+		kfree(vkms_configfs_crtc);
+		mutex_unlock(&vkms_configfs->lock);
+		return ERR_PTR(-ENOMEM);
+	}
+
+	vkms_configfs_crtc->vkms_configfs_device = vkms_configfs;
+
+	strscpy(vkms_configfs_crtc->vkms_config_crtc->name, name, strlen(name) + 1);
+	config_group_init_type_name(&vkms_configfs_crtc->group, name,
+				    &crtc_item_type);
+
+	mutex_unlock(&vkms_configfs->lock);
+
+	return &vkms_configfs_crtc->group;
+}
+
+static struct configfs_group_operations crtcs_group_operations = {
+	.make_group	= &crtcs_make_group,
+};
+
+static const struct config_item_type crtcs_item_type = {
+	.ct_group_ops    = &crtcs_group_operations,
+	.ct_owner        = THIS_MODULE,
+};
+
+static int encoder_possible_crtcs_allow_link(struct config_item *src,
+					     struct config_item *target)
+{
+	struct vkms_config_crtc *crtc;
+	struct vkms_configfs_device *vkms_configfs = encoder_possible_crtc_src_item_to_vkms_configfs_device(src);
+
+	mutex_lock(&vkms_configfs->lock);
+
+	if (target->ci_type != &crtc_item_type) {
+		DRM_ERROR("Unable to link non-CRTCs.\n");
+		mutex_unlock(&vkms_configfs->lock);
+		return -EINVAL;
+	}
+
+	crtc = crtc_item_to_vkms_configfs_crtc(target)->vkms_config_crtc;
+	struct vkms_config_encoder *encoder = encoder_possible_crtc_src_item_to_vkms_configfs_encoder(src)->vkms_config_encoder;
+
+	struct vkms_config_crtc *crtc_entry;
+	unsigned long idx = 0;
+
+	xa_for_each(&encoder->possible_crtcs, idx, crtc_entry) {
+		if (crtc_entry == crtc) {
+			pr_err("Tried to add two symlinks to the same CRTC from the same object.\n");
+			mutex_unlock(&vkms_configfs->lock);
+			return -EINVAL;
+		}
+	}
+
+	if (vkms_config_encoder_attach_crtc(encoder, crtc))
+		return -EINVAL;
+
+	mutex_unlock(&vkms_configfs->lock);
+
+	return 0;
+}
+
+static void encoder_possible_crtcs_drop_link(struct config_item *src,
+					     struct config_item *target)
+{
+	struct vkms_config_crtc *crtc;
+	struct vkms_configfs_device *vkms_configfs = encoder_possible_crtc_src_item_to_vkms_configfs_device(src);
+
+	mutex_lock(&vkms_configfs->lock);
+
+	crtc = crtc_item_to_vkms_configfs_crtc(target)->vkms_config_crtc;
+	struct vkms_config_encoder *encoder = encoder_possible_crtc_src_item_to_vkms_configfs_encoder(src)->vkms_config_encoder;
+
+	struct vkms_config_encoder *encoder_entry;
+	struct vkms_config_crtc *crtc_entry;
+	unsigned long encoder_idx = -1;
+	unsigned long crtc_idx = -1;
+
+	xa_for_each(&encoder->possible_crtcs, crtc_idx, crtc_entry) {
+		if (crtc_entry == crtc)
+			break;
+	}
+	xa_erase(&encoder->possible_crtcs, crtc_idx);
+	xa_for_each(&crtc->possible_encoders, encoder_idx, encoder_entry) {
+		if (encoder_entry == encoder)
+			break;
+	}
+	xa_erase(&crtc->possible_encoders, encoder_idx);
+
+	mutex_unlock(&vkms_configfs->lock);
+}
+
+static struct configfs_item_operations encoder_possible_crtcs_item_operations = {
+	.allow_link	= &encoder_possible_crtcs_allow_link,
+	.drop_link	= &encoder_possible_crtcs_drop_link,
+};
+
+static struct config_item_type encoder_possible_crtcs_item_type = {
+	.ct_item_ops	= &encoder_possible_crtcs_item_operations,
+	.ct_owner	= THIS_MODULE,
+};
+
+static void encoder_release(struct config_item *item)
+{
+	struct vkms_configfs_encoder *vkms_configfs_encoder = encoder_item_to_vkms_configfs_encoder(item);
+
+	mutex_lock(&vkms_configfs_encoder->vkms_configfs_device->lock);
+	vkms_config_delete_encoder(vkms_configfs_encoder->vkms_config_encoder,
+				   vkms_configfs_encoder->vkms_configfs_device->vkms_config);
+	mutex_unlock(&vkms_configfs_encoder->vkms_configfs_device->lock);
+
+	kfree(vkms_configfs_encoder);
+}
+
+static struct configfs_item_operations encoder_item_operations = {
+	.release	= encoder_release,
+};
+
+static const struct config_item_type encoder_item_type = {
+	.ct_item_ops	= &encoder_item_operations,
+	.ct_owner	= THIS_MODULE,
+};
+
+static struct config_group *encoder_make_group(struct config_group *config_group,
+					       const char *name)
+{
+	struct vkms_configfs_device *vkms_configfs = encoder_item_to_vkms_configfs_device(&config_group->cg_item);
+	struct vkms_configfs_encoder *vkms_configfs_encoder;
+
+	vkms_configfs_encoder = kzalloc(sizeof(*vkms_configfs_encoder), GFP_KERNEL);
+
+	if (!vkms_configfs_encoder)
+		return ERR_PTR(-ENOMEM);
+
+	mutex_lock(&vkms_configfs->lock);
+
+	if (vkms_configfs->enabled) {
+		kfree(vkms_configfs_encoder);
+		mutex_unlock(&vkms_configfs->lock);
+		return ERR_PTR(-EINVAL);
+	}
+
+	vkms_configfs_encoder->vkms_config_encoder = vkms_config_create_encoder(vkms_configfs->vkms_config);
+
+	if (!vkms_configfs_encoder->vkms_config_encoder) {
+		kfree(vkms_configfs_encoder);
+		mutex_unlock(&vkms_configfs->lock);
+		return ERR_PTR(-ENOMEM);
+	}
+
+	vkms_configfs_encoder->vkms_config_encoder->name = kzalloc(strlen(name) + 1, GFP_KERNEL);
+	if (!vkms_configfs_encoder->vkms_config_encoder->name) {
+		kfree(vkms_configfs_encoder->vkms_config_encoder);
+		kfree(vkms_configfs_encoder);
+		mutex_unlock(&vkms_configfs->lock);
+		return ERR_PTR(-ENOMEM);
+	}
+
+	strscpy(vkms_configfs_encoder->vkms_config_encoder->name, name, strlen(name) + 1);
+	config_group_init_type_name(&vkms_configfs_encoder->group, name,
+				    &encoder_item_type);
+
+	config_group_init_type_name(&vkms_configfs_encoder->possible_crtc_group, "possible_crtcs",
+				    &encoder_possible_crtcs_item_type);
+	configfs_add_default_group(&vkms_configfs_encoder->possible_crtc_group,
+				   &vkms_configfs_encoder->group);
+	vkms_configfs_encoder->vkms_configfs_device = vkms_configfs;
+
+	mutex_unlock(&vkms_configfs->lock);
+
+	return &vkms_configfs_encoder->group;
+}
+
+static struct configfs_group_operations encoder_group_operations = {
+	.make_group	= &encoder_make_group,
+};
+
+static const struct config_item_type encoders_item_type = {
+	.ct_group_ops	= &encoder_group_operations,
+	.ct_owner	= THIS_MODULE,
+};
+
+/**
+ * configfs_lock_dependencies() - In order to forbid the userspace to delete items when the
+ * device is enabled, mark all configfs items as dependent
+ *
+ * @vkms_configfs_device: Device to lock
+ */
+static void configfs_lock_dependencies(struct vkms_configfs_device *vkms_configfs_device)
+{
+	/* Lock the group itself */
+	configfs_depend_item_unlocked(vkms_configfs_device->group.cg_subsys,
+				      &vkms_configfs_device->group.cg_item);
+	/* Lock the planes elements */
+	struct config_item *item;
+
+	list_for_each_entry(item, &vkms_configfs_device->plane_group.cg_children, ci_entry) {
+		configfs_depend_item_unlocked(vkms_configfs_device->plane_group.cg_subsys,
+					      item);
+	}
+	list_for_each_entry(item, &vkms_configfs_device->crtc_group.cg_children, ci_entry) {
+		configfs_depend_item_unlocked(vkms_configfs_device->crtc_group.cg_subsys,
+					      item);
+	}
+}
+
+/**
+ * configfs_unlock_dependencies() - Once the device is disable, its configuration can be modified.
+ *
+ * @vkms_configfs_device: Device to unlock
+ */
+static void configfs_unlock_dependencies(struct vkms_configfs_device *vkms_configfs_device)
+{
+	struct config_item *item;
+
+	configfs_undepend_item_unlocked(&vkms_configfs_device->group.cg_item);
+
+	list_for_each_entry(item, &vkms_configfs_device->plane_group.cg_children, ci_entry) {
+		configfs_undepend_item_unlocked(item);
+	}
+	list_for_each_entry(item, &vkms_configfs_device->crtc_group.cg_children, ci_entry) {
+		configfs_undepend_item_unlocked(item);
+	}
+}
+
 static ssize_t device_enable_show(struct config_item *item, char *page)
 {
 	return sprintf(page, "%d\n",
@@ -474,13 +832,25 @@ static ssize_t device_enable_store(struct config_item *item,
 		return -EINVAL;
 
 	mutex_lock(&vkms_configfs_device->lock);
+	if (vkms_configfs_device->enabled == value) {
+		mutex_unlock(&vkms_configfs_device->lock);
+		return (ssize_t)count;
+	}
+
+	if (value && !vkms_config_is_valid(vkms_configfs_device->vkms_config)) {
+		mutex_unlock(&vkms_configfs_device->lock);
+		return -EINVAL;
+	}
 
 	vkms_configfs_device->enabled = value;
 
-	if (value)
+	if (value) {
+		configfs_lock_dependencies(vkms_configfs_device);
 		vkms_create(vkms_configfs_device->vkms_config);
-	else
+	} else {
+		configfs_unlock_dependencies(vkms_configfs_device);
 		vkms_destroy(vkms_configfs_device->vkms_config);
+	}
 
 	mutex_unlock(&vkms_configfs_device->lock);
 
@@ -519,9 +889,6 @@ static const struct config_item_type device_item_type = {
 static struct config_group *root_make_group(struct config_group *group,
 					    const char *name)
 {
-	struct vkms_config_plane *plane;
-	struct vkms_config_crtc *crtc;
-	struct vkms_config_encoder *encoder;
 	struct vkms_configfs_device *configfs = kzalloc(sizeof(*configfs), GFP_KERNEL);
 
 	if (!configfs)
@@ -536,22 +903,18 @@ static struct config_group *root_make_group(struct config_group *group,
 		return ERR_PTR(-ENOMEM);
 	}
 
-	configfs->vkms_config_crtc = vkms_config_create_crtc(configfs->vkms_config);
-	configfs->vkms_config_encoder = vkms_config_create_encoder(configfs->vkms_config);
-	if (!configfs->vkms_config_crtc || !configfs->vkms_config_encoder ||
-	    vkms_config_encoder_attach_crtc(configfs->vkms_config_encoder,
-					    configfs->vkms_config_crtc)) {
-		vkms_config_destroy(configfs->vkms_config);
-		kfree(configfs);
-		return ERR_PTR(-ENOMEM);
-	}
-
 	config_group_init_type_name(&configfs->group, name,
 				    &device_item_type);
 
 	config_group_init_type_name(&configfs->plane_group, "planes", &planes_item_type);
 	configfs_add_default_group(&configfs->plane_group, &configfs->group);
 
+	config_group_init_type_name(&configfs->crtc_group, "crtcs", &crtcs_item_type);
+	configfs_add_default_group(&configfs->crtc_group, &configfs->group);
+
+	config_group_init_type_name(&configfs->encoder_group, "encoders", &encoders_item_type);
+	configfs_add_default_group(&configfs->encoder_group, &configfs->group);
+
 	return &configfs->group;
 }
 
diff --git a/drivers/gpu/drm/vkms/vkms_configfs.h b/drivers/gpu/drm/vkms/vkms_configfs.h
index 6dc4d34a9e44ed4beb115ad3c86b759e28f5d0ef..df743e0107f40cd10433bdb638108d266f9c83a6 100644
--- a/drivers/gpu/drm/vkms/vkms_configfs.h
+++ b/drivers/gpu/drm/vkms/vkms_configfs.h
@@ -20,34 +20,84 @@ struct vkms_configfs_device {
 	struct config_group group;
 
 	struct config_group plane_group;
+	struct config_group crtc_group;
+	struct config_group encoder_group;
 
 	struct mutex lock;
 	bool enabled;
 
 	struct vkms_config *vkms_config;
-	struct vkms_config_crtc *vkms_config_crtc;
-	struct vkms_config_encoder *vkms_config_encoder;
 };
 
 struct vkms_configfs_plane {
 	struct config_group group;
+	struct config_group possible_crtc_group;
 
 	struct vkms_configfs_device *vkms_configfs_device;
 	struct vkms_config_plane *vkms_config_plane;
 };
 
+struct vkms_configfs_crtc {
+	struct config_group group;
+
+	struct vkms_configfs_device *vkms_configfs_device;
+	struct vkms_config_crtc *vkms_config_crtc;
+};
+
+struct vkms_configfs_encoder {
+	/* must be first because it is a krefcounted stuff */
+	struct config_group group;
+
+	struct config_group possible_crtc_group;
+	struct vkms_configfs_device *vkms_configfs_device;
+	struct vkms_config_encoder *vkms_config_encoder;
+};
+
 #define config_item_to_vkms_configfs_device(item) \
 	container_of(to_config_group((item)), struct vkms_configfs_device, group)
 
 #define planes_item_to_vkms_configfs_device(item) \
 	config_item_to_vkms_configfs_device((item)->ci_parent)
 
+#define encoders_item_to_vkms_configfs_device(item) \
+	config_item_to_vkms_configfs_device((item)->ci_parent)
+
+#define crtc_item_to_vkms_configfs_crtc(item) \
+	container_of(to_config_group((item)), struct vkms_configfs_crtc, group)
+
+#define encoder_item_to_vkms_configfs_encoder(item) \
+	container_of(to_config_group((item)), struct vkms_configfs_encoder, group)
+
 #define plane_item_to_vkms_configfs_device(item) \
 	planes_item_to_vkms_configfs_device((item)->ci_parent)
 
 #define plane_item_to_vkms_configfs_plane(item) \
 	container_of(to_config_group((item)), struct vkms_configfs_plane, group)
 
+#define plane_possible_crtc_src_item_to_vkms_configfs_device(item) \
+	plane_item_to_vkms_configfs_device((item)->ci_parent)
+
+#define plane_possible_crtc_src_item_to_vkms_configfs_plane(item) \
+	plane_item_to_vkms_configfs_plane((item)->ci_parent)
+
+#define crtc_item_to_vkms_configfs_device(item) \
+	config_item_to_vkms_configfs_device((item)->ci_parent)
+
+#define crtc_child_item_to_vkms_configfs_device(item) \
+	crtc_item_to_vkms_configfs_device((item)->ci_parent)
+
+#define encoder_item_to_vkms_configfs_device(item) \
+	config_item_to_vkms_configfs_device((item)->ci_parent)
+
+#define encoder_child_item_to_vkms_configfs_device(item) \
+	encoder_item_to_vkms_configfs_device((item)->ci_parent)
+
+#define encoder_possible_crtc_src_item_to_vkms_configfs_device(item) \
+	encoder_child_item_to_vkms_configfs_device((item)->ci_parent)
+
+#define encoder_possible_crtc_src_item_to_vkms_configfs_encoder(item) \
+	encoder_item_to_vkms_configfs_encoder((item)->ci_parent)
+
 /* ConfigFS Support */
 int vkms_init_configfs(void);
 void vkms_unregister_configfs(void);

-- 
2.47.0

[PATCH RFC v2 09/16] drm/vkms: Introduce configfs for connectors

[Louis Chauvet]

Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
---
 drivers/gpu/drm/vkms/vkms_config.c   |  13 ++++
 drivers/gpu/drm/vkms/vkms_config.h   |   3 +
 drivers/gpu/drm/vkms/vkms_configfs.c | 138 +++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/vkms/vkms_configfs.h |  23 ++++++
 4 files changed, 177 insertions(+)

diff --git a/drivers/gpu/drm/vkms/vkms_config.c b/drivers/gpu/drm/vkms/vkms_config.c
index 9a461a0481c2a20d6d48f1aa9649843ad1b7d13d..da99785ec89f0c6a7fe1a71fd2e6f5944c844aa9 100644
--- a/drivers/gpu/drm/vkms/vkms_config.c
+++ b/drivers/gpu/drm/vkms/vkms_config.c
@@ -516,6 +516,19 @@ vkms_config_connector_attach_encoder(struct vkms_config_connector *vkms_config_c
 	return ret;
 }
 
+void vkms_config_connector_detach_encoder(struct vkms_config_connector *vkms_config_connector,
+					  struct vkms_config_encoder *vkms_config_encoder)
+{
+	struct vkms_config_encoder *encoder_entry;
+	unsigned long encoder_idx;
+
+	xa_for_each(&vkms_config_connector->possible_encoders, encoder_idx, encoder_entry) {
+		if (encoder_entry == vkms_config_encoder)
+			break;
+	}
+	xa_erase(&vkms_config_connector->possible_encoders, encoder_idx);
+}
+
 bool vkms_config_is_valid(struct vkms_config *config)
 {
 	struct vkms_config_plane *config_plane;
diff --git a/drivers/gpu/drm/vkms/vkms_config.h b/drivers/gpu/drm/vkms/vkms_config.h
index 529d9c99f3c406d49dc7f3689a84c3dd775399a9..b1d80185216798cc9fc06e7d1cd0c423b7275185 100644
--- a/drivers/gpu/drm/vkms/vkms_config.h
+++ b/drivers/gpu/drm/vkms/vkms_config.h
@@ -197,6 +197,9 @@ int __must_check vkms_config_encoder_attach_crtc(struct vkms_config_encoder *vkm
 int __must_check
 vkms_config_connector_attach_encoder(struct vkms_config_connector *vkms_config_connector,
 				     struct vkms_config_encoder *vkms_config_encoder);
+void vkms_config_connector_detach_encoder(struct vkms_config_connector *vkms_config_connector,
+					  struct vkms_config_encoder *vkms_config_encoder);
+
 /**
  * vkms_config_delete_plane() - Remove a plane configuration and frees its memory
  *
diff --git a/drivers/gpu/drm/vkms/vkms_configfs.c b/drivers/gpu/drm/vkms/vkms_configfs.c
index a410c9be4f2bbf7b2651245747eb357fcf32d1f2..94c288514172b88d06c2b74e36569c6d55383782 100644
--- a/drivers/gpu/drm/vkms/vkms_configfs.c
+++ b/drivers/gpu/drm/vkms/vkms_configfs.c
@@ -746,6 +746,7 @@ static struct config_group *encoder_make_group(struct config_group *config_group
 	}
 
 	strscpy(vkms_configfs_encoder->vkms_config_encoder->name, name, strlen(name) + 1);
+
 	config_group_init_type_name(&vkms_configfs_encoder->group, name,
 				    &encoder_item_type);
 
@@ -769,6 +770,139 @@ static const struct config_item_type encoders_item_type = {
 	.ct_owner	= THIS_MODULE,
 };
 
+static int connector_possible_encoders_allow_link(struct config_item *src,
+						  struct config_item *target)
+{
+	struct vkms_config_encoder *vkms_config_encoder;
+	struct vkms_configfs_device *vkms_configfs =
+		connector_possible_encoder_src_item_to_vkms_configfs_device
+		(src);
+
+	mutex_lock(&vkms_configfs->lock);
+
+	if (target->ci_type != &encoder_item_type) {
+		DRM_ERROR("Unable to link non-CRTCs.\n");
+		mutex_unlock(&vkms_configfs->lock);
+		return -EINVAL;
+	}
+
+	vkms_config_encoder = encoder_item_to_vkms_configfs_encoder(target)
+				      ->vkms_config_encoder;
+	struct vkms_config_connector *vkms_config_connector =
+		connector_possible_encoder_src_item_to_vkms_configfs_connector
+		(src)
+			->vkms_config_connector;
+
+	if (vkms_config_connector_attach_encoder(vkms_config_connector,
+						 vkms_config_encoder))
+		return -EINVAL;
+
+	mutex_unlock(&vkms_configfs->lock);
+
+	return 0;
+}
+
+static void connector_possible_encoders_drop_link(struct config_item *src,
+						  struct config_item *target)
+{
+	struct vkms_config_encoder *vkms_config_encoder;
+	struct vkms_configfs_device *vkms_configfs =
+		connector_possible_encoder_src_item_to_vkms_configfs_device(src);
+
+	mutex_lock(&vkms_configfs->lock);
+
+	vkms_config_encoder = encoder_item_to_vkms_configfs_encoder(target)->vkms_config_encoder;
+	struct vkms_config_connector *vkms_config_connector =
+		connector_possible_encoder_src_item_to_vkms_configfs_connector(src)
+			->vkms_config_connector;
+
+	vkms_config_connector_detach_encoder(vkms_config_connector, vkms_config_encoder);
+
+	mutex_unlock(&vkms_configfs->lock);
+}
+
+static struct configfs_item_operations connector_possible_encoders_item_operations = {
+	.allow_link = &connector_possible_encoders_allow_link,
+	.drop_link = &connector_possible_encoders_drop_link,
+};
+
+static struct config_item_type connector_possible_encoders_item_type = {
+	.ct_item_ops = &connector_possible_encoders_item_operations,
+	.ct_owner = THIS_MODULE,
+};
+
+static void connector_release(struct config_item *item)
+{
+	struct vkms_configfs_connector *vkms_configfs_connector =
+		connector_item_to_vkms_configfs_connector(item);
+
+	mutex_lock(&vkms_configfs_connector->vkms_configfs_device->lock);
+	vkms_config_delete_connector(vkms_configfs_connector->vkms_config_connector);
+	mutex_unlock(&vkms_configfs_connector->vkms_configfs_device->lock);
+
+	kfree(vkms_configfs_connector);
+}
+
+static struct configfs_item_operations connector_item_operations = {
+	.release = connector_release,
+};
+
+static const struct config_item_type connector_item_type = {
+	.ct_item_ops = &connector_item_operations,
+	.ct_owner = THIS_MODULE,
+};
+
+static struct config_group *connector_make_group(struct config_group *config_group,
+						 const char *name)
+{
+	struct vkms_configfs_device *vkms_configfs =
+		connector_item_to_vkms_configfs_device(&config_group->cg_item);
+	struct vkms_configfs_connector *vkms_configfs_connector;
+
+	vkms_configfs_connector = kzalloc(sizeof(*vkms_configfs_connector), GFP_KERNEL);
+
+	if (!vkms_configfs_connector)
+		return ERR_PTR(-ENOMEM);
+
+	mutex_lock(&vkms_configfs->lock);
+
+	if (vkms_configfs->enabled) {
+		kfree(vkms_configfs_connector);
+		mutex_unlock(&vkms_configfs->lock);
+		return ERR_PTR(-EINVAL);
+	}
+
+	vkms_configfs_connector->vkms_config_connector =
+		vkms_config_create_connector(vkms_configfs->vkms_config);
+
+	if (!vkms_configfs_connector->vkms_config_connector) {
+		kfree(vkms_configfs_connector);
+		mutex_unlock(&vkms_configfs->lock);
+		return ERR_PTR(-ENOMEM);
+	}
+
+	config_group_init_type_name(&vkms_configfs_connector->group, name, &connector_item_type);
+
+	config_group_init_type_name(&vkms_configfs_connector->possible_encoder_group,
+				    "possible_encoders", &connector_possible_encoders_item_type);
+	configfs_add_default_group(&vkms_configfs_connector->possible_encoder_group,
+				   &vkms_configfs_connector->group);
+	vkms_configfs_connector->vkms_configfs_device = vkms_configfs;
+
+	mutex_unlock(&vkms_configfs->lock);
+
+	return &vkms_configfs_connector->group;
+}
+
+static struct configfs_group_operations connector_group_operations = {
+	.make_group = &connector_make_group,
+};
+
+static const struct config_item_type connectors_item_type = {
+	.ct_group_ops = &connector_group_operations,
+	.ct_owner = THIS_MODULE,
+};
+
 /**
  * configfs_lock_dependencies() - In order to forbid the userspace to delete items when the
  * device is enabled, mark all configfs items as dependent
@@ -915,6 +1049,10 @@ static struct config_group *root_make_group(struct config_group *group,
 	config_group_init_type_name(&configfs->encoder_group, "encoders", &encoders_item_type);
 	configfs_add_default_group(&configfs->encoder_group, &configfs->group);
 
+	config_group_init_type_name(&configfs->connector_group, "connectors",
+				    &connectors_item_type);
+	configfs_add_default_group(&configfs->connector_group, &configfs->group);
+
 	return &configfs->group;
 }
 
diff --git a/drivers/gpu/drm/vkms/vkms_configfs.h b/drivers/gpu/drm/vkms/vkms_configfs.h
index df743e0107f40cd10433bdb638108d266f9c83a6..12c0fdefb813387515d144519479c242b7ef6728 100644
--- a/drivers/gpu/drm/vkms/vkms_configfs.h
+++ b/drivers/gpu/drm/vkms/vkms_configfs.h
@@ -22,6 +22,7 @@ struct vkms_configfs_device {
 	struct config_group plane_group;
 	struct config_group crtc_group;
 	struct config_group encoder_group;
+	struct config_group connector_group;
 
 	struct mutex lock;
 	bool enabled;
@@ -53,6 +54,14 @@ struct vkms_configfs_encoder {
 	struct vkms_config_encoder *vkms_config_encoder;
 };
 
+struct vkms_configfs_connector {
+	struct config_group group;
+
+	struct config_group possible_encoder_group;
+	struct vkms_configfs_device *vkms_configfs_device;
+	struct vkms_config_connector *vkms_config_connector;
+};
+
 #define config_item_to_vkms_configfs_device(item) \
 	container_of(to_config_group((item)), struct vkms_configfs_device, group)
 
@@ -68,6 +77,9 @@ struct vkms_configfs_encoder {
 #define encoder_item_to_vkms_configfs_encoder(item) \
 	container_of(to_config_group((item)), struct vkms_configfs_encoder, group)
 
+#define connector_item_to_vkms_configfs_connector(item) \
+	container_of(to_config_group((item)), struct vkms_configfs_connector, group)
+
 #define plane_item_to_vkms_configfs_device(item) \
 	planes_item_to_vkms_configfs_device((item)->ci_parent)
 
@@ -89,14 +101,25 @@ struct vkms_configfs_encoder {
 #define encoder_item_to_vkms_configfs_device(item) \
 	config_item_to_vkms_configfs_device((item)->ci_parent)
 
+#define connector_item_to_vkms_configfs_device(item) \
+	config_item_to_vkms_configfs_device((item)->ci_parent)
+
 #define encoder_child_item_to_vkms_configfs_device(item) \
 	encoder_item_to_vkms_configfs_device((item)->ci_parent)
 
 #define encoder_possible_crtc_src_item_to_vkms_configfs_device(item) \
 	encoder_child_item_to_vkms_configfs_device((item)->ci_parent)
 
+#define connector_child_item_to_vkms_configfs_device(item) \
+	connector_item_to_vkms_configfs_device((item)->ci_parent)
+
+#define connector_possible_encoder_src_item_to_vkms_configfs_device(item) \
+	connector_child_item_to_vkms_configfs_device((item)->ci_parent)
+
 #define encoder_possible_crtc_src_item_to_vkms_configfs_encoder(item) \
 	encoder_item_to_vkms_configfs_encoder((item)->ci_parent)
+#define connector_possible_encoder_src_item_to_vkms_configfs_connector(item) \
+	connector_item_to_vkms_configfs_connector((item)->ci_parent)
 
 /* ConfigFS Support */
 int vkms_init_configfs(void);

-- 
2.47.0

[PATCH RFC v2 10/16] drm/vkms: Introduce configfs for connector type

[Louis Chauvet]

Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
---
 drivers/gpu/drm/vkms/vkms_configfs.c | 71 ++++++++++++++++++++++++++++++++++++
 1 file changed, 71 insertions(+)

diff --git a/drivers/gpu/drm/vkms/vkms_configfs.c b/drivers/gpu/drm/vkms/vkms_configfs.c
index 94c288514172b88d06c2b74e36569c6d55383782..ef9a00c41b7b030d0d60ffe078809f9799539f40 100644
--- a/drivers/gpu/drm/vkms/vkms_configfs.c
+++ b/drivers/gpu/drm/vkms/vkms_configfs.c
@@ -843,12 +843,83 @@ static void connector_release(struct config_item *item)
 	kfree(vkms_configfs_connector);
 }
 
+static ssize_t connector_type_show(struct config_item *item, char *page)
+{
+	struct vkms_config_connector *connector;
+	int connector_type;
+	struct vkms_configfs_device *vkms_configfs = connector_child_item_to_vkms_configfs_device(item);
+
+	mutex_lock(&vkms_configfs->lock);
+	connector = connector_item_to_vkms_configfs_connector(item)->vkms_config_connector;
+	connector_type = connector->type;
+	mutex_unlock(&vkms_configfs->lock);
+
+	return sprintf(page, "%u", connector_type);
+}
+
+static ssize_t connector_type_store(struct config_item *item,
+				    const char *page, size_t count)
+{
+	struct vkms_config_connector *connector;
+	int val = DRM_MODE_CONNECTOR_VIRTUAL;
+	struct vkms_configfs_device *vkms_configfs = connector_child_item_to_vkms_configfs_device(item);
+	int ret;
+
+	ret = kstrtouint(page, 10, &val);
+	if (ret)
+		return ret;
+
+	switch (val) {
+	case DRM_MODE_CONNECTOR_Unknown:
+	case DRM_MODE_CONNECTOR_VGA:
+	case DRM_MODE_CONNECTOR_DVII:
+	case DRM_MODE_CONNECTOR_DVID:
+	case DRM_MODE_CONNECTOR_DVIA:
+	case DRM_MODE_CONNECTOR_Composite:
+	case DRM_MODE_CONNECTOR_SVIDEO:
+	case DRM_MODE_CONNECTOR_LVDS:
+	case DRM_MODE_CONNECTOR_Component:
+	case DRM_MODE_CONNECTOR_9PinDIN:
+	case DRM_MODE_CONNECTOR_DisplayPort:
+	case DRM_MODE_CONNECTOR_HDMIA:
+	case DRM_MODE_CONNECTOR_HDMIB:
+	case DRM_MODE_CONNECTOR_TV:
+	case DRM_MODE_CONNECTOR_eDP:
+	case DRM_MODE_CONNECTOR_VIRTUAL:
+	case DRM_MODE_CONNECTOR_DSI:
+	case DRM_MODE_CONNECTOR_DPI:
+	case DRM_MODE_CONNECTOR_SPI:
+	case DRM_MODE_CONNECTOR_USB:
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	scoped_guard(mutex, &vkms_configfs->lock) {
+		if (vkms_configfs->enabled)
+			return -EINVAL;
+
+		connector = connector_item_to_vkms_configfs_connector(item)->vkms_config_connector;
+		connector->type = val;
+	}
+
+	return count;
+}
+
+CONFIGFS_ATTR(connector_, type);
+
+static struct configfs_attribute *connector_attrs[] = {
+	&connector_attr_type,
+	NULL,
+};
+
 static struct configfs_item_operations connector_item_operations = {
 	.release = connector_release,
 };
 
 static const struct config_item_type connector_item_type = {
 	.ct_item_ops = &connector_item_operations,
+	.ct_attrs = connector_attrs,
 	.ct_owner = THIS_MODULE,
 };
 

-- 
2.47.0

[PATCH RFC v2 11/16] drm/vkms: Introduce configfs for plane format

[Louis Chauvet]

Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
---
 drivers/gpu/drm/vkms/vkms_configfs.c | 75 ++++++++++++++++++++++++++++++++++++
 1 file changed, 75 insertions(+)

diff --git a/drivers/gpu/drm/vkms/vkms_configfs.c b/drivers/gpu/drm/vkms/vkms_configfs.c
index ef9a00c41b7b030d0d60ffe078809f9799539f40..a1ddf25ab6f882ae1857bc82fb4b4a425b6d287d 100644
--- a/drivers/gpu/drm/vkms/vkms_configfs.c
+++ b/drivers/gpu/drm/vkms/vkms_configfs.c
@@ -355,6 +355,79 @@ static ssize_t plane_default_color_encoding_store(struct config_item *item,
 	return count;
 }
 
+static ssize_t plane_supported_formats_show(struct config_item *item, char *page)
+{
+	struct vkms_config_plane *plane = plane_item_to_vkms_configfs_plane(item)->vkms_config_plane;
+	struct vkms_configfs_device *vkms_configfs = plane_item_to_vkms_configfs_device(item);
+
+	page[0] = '\0';
+
+	scoped_guard(mutex, &vkms_configfs->lock)
+	{
+		for (int i = 0; i < plane->supported_formats_count; i++) {
+			char tmp[6] = { 0 };
+			const ssize_t ret = snprintf(tmp, ARRAY_SIZE(tmp), "%.*s\n",
+					       (int)sizeof(plane->supported_formats[i]),
+					       (char *)&plane->supported_formats[i]);
+			if (ret < 0)
+				return ret;
+			/* Limitation of ConfigFS attributes, an attribute can't be bigger than PAGE_SIZE */
+			if (ret + strlen(page) > PAGE_SIZE - 1)
+				return -ENOMEM;
+			strncat(page, tmp, ARRAY_SIZE(tmp));
+		}
+	}
+
+	return strlen(page);
+}
+
+static ssize_t plane_supported_formats_store(struct config_item *item,
+					     const char *page, size_t count)
+{
+	struct vkms_config_plane *plane = plane_item_to_vkms_configfs_plane(item)->vkms_config_plane;
+	struct vkms_configfs_device *vkms_configfs = plane_item_to_vkms_configfs_device(item);
+	int ret = 0;
+	int ptr = 0;
+
+	scoped_guard(mutex, &vkms_configfs->lock)
+	{
+		while (ptr < count) {
+			if (page[ptr] == '+') {
+				char tmp[4] = { ' ', ' ', ' ', ' ' };
+
+				memcpy(tmp, &page[ptr + 1], min(sizeof(tmp), count - (ptr + 1)));
+				if (tmp[0] == '*') {
+					ret = vkms_config_plane_add_all_formats(plane);
+					if (ret)
+						return ret;
+					ptr += 1;
+				} else {
+					ret = vkms_config_plane_add_format(plane, *(int *)tmp);
+					if (ret)
+						return ret;
+					ptr += 4;
+				}
+			} else if (page[ptr] == '-') {
+				char tmp[4] = { ' ', ' ', ' ', ' ' };
+
+				memcpy(tmp, &page[ptr + 1], min(sizeof(tmp), count - (ptr + 1)));
+				if (tmp[0] == '*') {
+					vkms_config_plane_remove_all_formats(plane);
+					ptr += 1;
+				} else {
+					vkms_config_plane_remove_format(plane, *(int *)tmp);
+
+					ptr += 4;
+				}
+			}
+			/* Skip anything that is not a + or a - */
+			ptr += 1;
+		}
+	}
+
+	return count;
+}
+
 CONFIGFS_ATTR(plane_, type);
 CONFIGFS_ATTR(plane_, supported_rotations);
 CONFIGFS_ATTR(plane_, default_rotation);
@@ -362,6 +435,7 @@ CONFIGFS_ATTR(plane_, color_range);
 CONFIGFS_ATTR(plane_, default_color_range);
 CONFIGFS_ATTR(plane_, supported_color_encoding);
 CONFIGFS_ATTR(plane_, default_color_encoding);
+CONFIGFS_ATTR(plane_, supported_formats);
 
 static struct configfs_attribute *plane_attrs[] = {
 	&plane_attr_type,
@@ -371,6 +445,7 @@ static struct configfs_attribute *plane_attrs[] = {
 	&plane_attr_default_color_range,
 	&plane_attr_supported_color_encoding,
 	&plane_attr_default_color_encoding,
+	&plane_attr_supported_formats,
 	NULL,
 };
 

-- 
2.47.0

[PATCH RFC v2 12/16] drm/vkms: Introduce configfs for device name

[Louis Chauvet]

Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
---
 drivers/gpu/drm/vkms/vkms_configfs.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/gpu/drm/vkms/vkms_configfs.c b/drivers/gpu/drm/vkms/vkms_configfs.c
index a1ddf25ab6f882ae1857bc82fb4b4a425b6d287d..dede56a8a2c1e866734f9c979c76977370907680 100644
--- a/drivers/gpu/drm/vkms/vkms_configfs.c
+++ b/drivers/gpu/drm/vkms/vkms_configfs.c
@@ -1137,10 +1137,26 @@ static ssize_t device_enable_store(struct config_item *item,
 	return (ssize_t)count;
 }
 
+static ssize_t device_device_name_show(struct config_item *item, char *page)
+{
+	struct vkms_configfs_device *configfs_device = config_item_to_vkms_configfs_device(item);
+
+	scoped_guard(mutex, &configfs_device->lock)
+	{
+		if (configfs_device->enabled)
+			return sprintf(page, "%s\n",
+				       dev_name(configfs_device->vkms_config->dev->drm.dev));
+		return -EINVAL;
+	}
+	return -EINVAL;
+}
+
 CONFIGFS_ATTR(device_, enable);
+CONFIGFS_ATTR_RO(device_, device_name);
 
 static struct configfs_attribute *device_attrs[] = {
 	&device_attr_enable,
+	&device_attr_device_name,
 	NULL,
 };
 

-- 
2.47.0

[PATCH RFC v2 13/16] drm/vkms: Introduce configfs for connector status

[Louis Chauvet]

Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
---
 drivers/gpu/drm/vkms/vkms_configfs.c | 45 ++++++++++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)

diff --git a/drivers/gpu/drm/vkms/vkms_configfs.c b/drivers/gpu/drm/vkms/vkms_configfs.c
index dede56a8a2c1e866734f9c979c76977370907680..6412a8b7b1066f2d3dca1135ebd7fbaed84027b6 100644
--- a/drivers/gpu/drm/vkms/vkms_configfs.c
+++ b/drivers/gpu/drm/vkms/vkms_configfs.c
@@ -981,10 +981,55 @@ static ssize_t connector_type_store(struct config_item *item,
 	return count;
 }
 
+static ssize_t connector_status_show(struct config_item *item, char *page)
+{
+	struct vkms_config_connector *connector;
+	enum drm_connector_status status;
+	struct vkms_configfs_device *vkms_configfs = connector_child_item_to_vkms_configfs_device(item);
+
+	mutex_lock(&vkms_configfs->lock);
+	connector = connector_item_to_vkms_configfs_connector(item)->vkms_config_connector;
+	status = connector->status;
+	mutex_unlock(&vkms_configfs->lock);
+
+	return sprintf(page, "%u", status);
+}
+
+static ssize_t connector_status_store(struct config_item *item,
+				      const char *page, size_t count)
+{
+	struct vkms_config_connector *connector;
+	enum drm_connector_status status = connector_status_unknown;
+	struct vkms_configfs_device *vkms_configfs = connector_child_item_to_vkms_configfs_device(item);
+	int ret;
+
+	ret = kstrtouint(page, 10, &status);
+	if (ret)
+		return ret;
+
+	switch (status) {
+	case connector_status_unknown:
+	case connector_status_connected:
+	case connector_status_disconnected:
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	scoped_guard(mutex, &vkms_configfs->lock) {
+		connector = connector_item_to_vkms_configfs_connector(item)->vkms_config_connector;
+		vkms_config_connector_update_status(connector, status);
+	}
+
+	return count;
+}
+
 CONFIGFS_ATTR(connector_, type);
+CONFIGFS_ATTR(connector_, status);
 
 static struct configfs_attribute *connector_attrs[] = {
 	&connector_attr_type,
+	&connector_attr_status,
 	NULL,
 };
 

-- 
2.47.0

[PATCH RFC v2 14/16] drm/vkms: Introduce configfs for connector id

[Louis Chauvet]

Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
---
 drivers/gpu/drm/vkms/vkms_configfs.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/drivers/gpu/drm/vkms/vkms_configfs.c b/drivers/gpu/drm/vkms/vkms_configfs.c
index 6412a8b7b1066f2d3dca1135ebd7fbaed84027b6..45738707f1f6c11b561e695fe0aef245d946b418 100644
--- a/drivers/gpu/drm/vkms/vkms_configfs.c
+++ b/drivers/gpu/drm/vkms/vkms_configfs.c
@@ -1024,12 +1024,31 @@ static ssize_t connector_status_store(struct config_item *item,
 	return count;
 }
 
+static ssize_t connector_id_show(struct config_item *item, char *page)
+{
+	struct vkms_configfs_device *vkms_configfs =
+		connector_child_item_to_vkms_configfs_device(item);
+	struct vkms_config_connector *connector;
+
+	scoped_guard(mutex, &vkms_configfs->lock)
+	{
+		connector = connector_item_to_vkms_configfs_connector(item)->vkms_config_connector;
+		if (vkms_configfs->enabled)
+			return sprintf(page, "%d\n",
+				       connector->connector->base.id);
+		return -EINVAL;
+	}
+	return -EINVAL;
+}
+
 CONFIGFS_ATTR(connector_, type);
 CONFIGFS_ATTR(connector_, status);
+CONFIGFS_ATTR_RO(connector_, id);
 
 static struct configfs_attribute *connector_attrs[] = {
 	&connector_attr_type,
 	&connector_attr_status,
+	&connector_attr_id,
 	NULL,
 };
 

-- 
2.47.0

[PATCH RFC v2 15/16] drm/vkms: Introduce configfs for connector EDID

[Louis Chauvet]

Due to limitation of ConfigFS, the max len of EDID is PAGE_SIZE (4kB on
x86), it should be sufficient for many tests. One possible evolution is
using a ConfigFS blob to allow bigger EDID.

The EDID can be changed at any time. As for physical display, you need
to trigger an HPD event to refresh the modes.

Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
---
 drivers/gpu/drm/vkms/vkms_configfs.c | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/drivers/gpu/drm/vkms/vkms_configfs.c b/drivers/gpu/drm/vkms/vkms_configfs.c
index 45738707f1f6c11b561e695fe0aef245d946b418..55bd0b9a31d35eca49c67a2fe2d58e3ea84367a4 100644
--- a/drivers/gpu/drm/vkms/vkms_configfs.c
+++ b/drivers/gpu/drm/vkms/vkms_configfs.c
@@ -1024,6 +1024,37 @@ static ssize_t connector_status_store(struct config_item *item,
 	return count;
 }
 
+static ssize_t connector_edid_show(struct config_item *item, char *page)
+{
+	struct vkms_config_connector *connector;
+	struct vkms_configfs_device *vkms_configfs = connector_child_item_to_vkms_configfs_device(item);
+
+	scoped_guard(mutex, &vkms_configfs->lock)
+	{
+		connector = connector_item_to_vkms_configfs_connector(item)->vkms_config_connector;
+		memcpy(page, &connector->edid_blob, connector->edid_blob_len);
+		return connector->edid_blob_len;
+	}
+
+	return -EINVAL;
+}
+
+static ssize_t connector_edid_store(struct config_item *item,
+				    const char *page, size_t count)
+{
+	struct vkms_configfs_device *vkms_configfs = connector_child_item_to_vkms_configfs_device(item);
+
+	scoped_guard(mutex, &vkms_configfs->lock)
+	{
+		struct vkms_config_connector *connector =
+			connector_item_to_vkms_configfs_connector(item)->vkms_config_connector;
+		memcpy(&connector->edid_blob, page, count);
+		connector->edid_blob_len = count;
+	}
+
+	return count;
+}
+
 static ssize_t connector_id_show(struct config_item *item, char *page)
 {
 	struct vkms_configfs_device *vkms_configfs =
@@ -1043,12 +1074,14 @@ static ssize_t connector_id_show(struct config_item *item, char *page)
 
 CONFIGFS_ATTR(connector_, type);
 CONFIGFS_ATTR(connector_, status);
+CONFIGFS_ATTR(connector_, edid);
 CONFIGFS_ATTR_RO(connector_, id);
 
 static struct configfs_attribute *connector_attrs[] = {
 	&connector_attr_type,
 	&connector_attr_status,
 	&connector_attr_id,
+	&connector_attr_edid,
 	NULL,
 };
 

-- 
2.47.0

[PATCH RFC v2 16/16] drm/vkms: Introduce configfs for encoder type

[Louis Chauvet]

Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
---
 drivers/gpu/drm/vkms/vkms_configfs.c | 57 ++++++++++++++++++++++++++++++++++++
 1 file changed, 57 insertions(+)

diff --git a/drivers/gpu/drm/vkms/vkms_configfs.c b/drivers/gpu/drm/vkms/vkms_configfs.c
index 55bd0b9a31d35eca49c67a2fe2d58e3ea84367a4..bfd3c3a0851b2cc76da52b8a190511f22d3f9fbc 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_mode.h"
 #include <linux/configfs.h>
 #include <linux/mutex.h>
 #include <drm/drm_print.h>
@@ -754,6 +755,61 @@ static void encoder_possible_crtcs_drop_link(struct config_item *src,
 	mutex_unlock(&vkms_configfs->lock);
 }
 
+static ssize_t encoder_type_show(struct config_item *item, char *page)
+{
+	struct vkms_config_encoder *encoder;
+	char encoder_type = DRM_MODE_ENCODER_NONE;
+	struct vkms_configfs_device *vkms_configfs = encoder_item_to_vkms_configfs_device(item);
+
+	scoped_guard(mutex, &vkms_configfs->lock)
+	{
+		encoder = encoder_item_to_vkms_configfs_encoder(item)->vkms_config_encoder;
+		encoder_type = encoder->type;
+	}
+
+	return sprintf(page, "%u", encoder_type);
+}
+
+static ssize_t encoder_type_store(struct config_item *item,
+				  const char *page, size_t count)
+{
+	struct vkms_configfs_device *vkms_configfs = encoder_item_to_vkms_configfs_device(item);
+	int val = DRM_MODE_ENCODER_VIRTUAL;
+	struct vkms_config_encoder *encoder;
+	int ret;
+
+	ret = kstrtouint(page, 10, &val);
+	if (ret)
+		return ret;
+
+	if (val != DRM_MODE_ENCODER_DAC &&
+	    val != DRM_MODE_ENCODER_DPI &&
+	    val != DRM_MODE_ENCODER_DSI &&
+	    val != DRM_MODE_ENCODER_LVDS &&
+	    val != DRM_MODE_ENCODER_NONE &&
+	    val != DRM_MODE_ENCODER_TMDS &&
+	    val != DRM_MODE_ENCODER_TVDAC &&
+	    val != DRM_MODE_ENCODER_VIRTUAL)
+		return -EINVAL;
+
+	scoped_guard(mutex, &vkms_configfs->lock) {
+		if (vkms_configfs->enabled)
+			return -EINVAL;
+
+		encoder = encoder_item_to_vkms_configfs_encoder(item)->vkms_config_encoder;
+		encoder->type = val;
+	}
+
+	return count;
+}
+
+CONFIGFS_ATTR(encoder_, type);
+
+static struct configfs_attribute *encoder_attrs[] = {
+	&encoder_attr_type,
+	NULL,
+};
+
 static struct configfs_item_operations encoder_possible_crtcs_item_operations = {
 	.allow_link	= &encoder_possible_crtcs_allow_link,
 	.drop_link	= &encoder_possible_crtcs_drop_link,
@@ -782,6 +838,7 @@ static struct configfs_item_operations encoder_item_operations = {
 
 static const struct config_item_type encoder_item_type = {
 	.ct_item_ops	= &encoder_item_operations,
+	.ct_attrs       = encoder_attrs,
 	.ct_owner	= THIS_MODULE,
 };
 

-- 
2.47.0

Re: [PATCH RFC v2 00/16] drm/vkms: ConfigFS interface

[Louis Chauvet]

On 22/11/24 - 18:38, Louis Chauvet wrote:
> VKMS is manly used to test userspace program and its behavior. The current 
> implementation is not very configurable as you can only have one device, 
> with few specific planes.
> 
> This series aims to introduce a new interface, using ConfigFS, to create 
> and configure more devices. This will introduce:
> - Device creation
> - Plane creation
> - Plane configuration (type, color encoding, color range, rotations)
> - Encoder creation
> - CRTC creation
> - Linking between CRTC and planes/encoders
> 
> The proposition is:
> /config/vkms
> 	DEVICE_1
> 	┣━ enable
> 	┣━ writeback
> 	┣━ planes
> 	┃  ┣━ PLANE_1
> 	┃  ┃  ┣━ type
> 	┃  ┃  ┣━ supported_rotations
> 	┃  ┃  ┣━ color_range
> 	┃  ┃  ┣━ color_encoding
> 	┃  ┃  ┣━ default_color_encoding
> 	┃  ┃  ┣━ default_rotations
> 	┃  ┃  ┣━ default_color_range
> 	┃  ┃  ┗━ possible_crtcs
> 	┃  ┃     ┗━ >> /config/vkms/DEVICE_1/crtc/CRTC_1 
> 	┃  ┣━ PLANE_2
> 	┃  ┃  ┗━ ditto
> 	┃  ┗━ PLANE_3
> 	┃     ┗━ ditto
> 	┃
> 	┣━ encoders
> 	┃  ┣━ ENCODER_1
> 	┃  ┃  ┗━ possible_crtcs
> 	┃  ┃     ┗━ >> /config/vkms/DEVICE_1/crtc/CRTC_1
> 	┃  ┗━ ENCODER_2
> 	┃     ┗━ ditto
> 	┃
> 	┗━ crtc
> 	   ┗━ CRTC_1
> 	
> This interface aims to be extendable (new property can easly be added in 
> objects) and easy to use (objects are created simply by creating folders, 
> and configured by writing files).
> 
> This series depends on 
> https://lore.kernel.org/all/20241122-google-remove-crtc-index-from-parameter-v2-0-81540742535a@bootlin.com
> but as this is a bit complex to rebase, you can find a working branch 
> here:
> https://gitlab.freedesktop.org/louischauvet/kernel/-/tree/b4/vkms-configfs
> 
> Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>

Hi all,

I am also currently working on MST emulation for VKMS. If someone can read 
what I already did and at tell me if my implementation seems on the right 
track it could be nice.

The current status is not very advanced: I can emulate a mst HUB, but not 
a screen. I am currently working on properly emulating the HUB by using an 
other hub.

You can find the branch for this work here:
https://gitlab.freedesktop.org/louischauvet/kernel/-/tree/b4/vkms-mst

Thanks a lot,
Louis Chauvet

Re: [PATCH RFC v2 00/16] drm/vkms: ConfigFS interface

[Maxime Ripard]

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

Hi,

On Fri, Nov 22, 2024 at 06:44:18PM +0100, Louis Chauvet wrote:
> On 22/11/24 - 18:38, Louis Chauvet wrote:
> > VKMS is manly used to test userspace program and its behavior. The current 
> > implementation is not very configurable as you can only have one device, 
> > with few specific planes.
> > 
> > This series aims to introduce a new interface, using ConfigFS, to create 
> > and configure more devices. This will introduce:
> > - Device creation
> > - Plane creation
> > - Plane configuration (type, color encoding, color range, rotations)
> > - Encoder creation
> > - CRTC creation
> > - Linking between CRTC and planes/encoders
> > 
> > The proposition is:
> > /config/vkms
> > 	DEVICE_1
> > 	┣━ enable
> > 	┣━ writeback
> > 	┣━ planes
> > 	┃  ┣━ PLANE_1
> > 	┃  ┃  ┣━ type
> > 	┃  ┃  ┣━ supported_rotations
> > 	┃  ┃  ┣━ color_range
> > 	┃  ┃  ┣━ color_encoding
> > 	┃  ┃  ┣━ default_color_encoding
> > 	┃  ┃  ┣━ default_rotations
> > 	┃  ┃  ┣━ default_color_range
> > 	┃  ┃  ┗━ possible_crtcs
> > 	┃  ┃     ┗━ >> /config/vkms/DEVICE_1/crtc/CRTC_1 
> > 	┃  ┣━ PLANE_2
> > 	┃  ┃  ┗━ ditto
> > 	┃  ┗━ PLANE_3
> > 	┃     ┗━ ditto
> > 	┃
> > 	┣━ encoders
> > 	┃  ┣━ ENCODER_1
> > 	┃  ┃  ┗━ possible_crtcs
> > 	┃  ┃     ┗━ >> /config/vkms/DEVICE_1/crtc/CRTC_1
> > 	┃  ┗━ ENCODER_2
> > 	┃     ┗━ ditto
> > 	┃
> > 	┗━ crtc
> > 	   ┗━ CRTC_1
> > 	
> > This interface aims to be extendable (new property can easly be added in 
> > objects) and easy to use (objects are created simply by creating folders, 
> > and configured by writing files).
> > 
> > This series depends on 
> > https://lore.kernel.org/all/20241122-google-remove-crtc-index-from-parameter-v2-0-81540742535a@bootlin.com
> > but as this is a bit complex to rebase, you can find a working branch 
> > here:
> > https://gitlab.freedesktop.org/louischauvet/kernel/-/tree/b4/vkms-configfs
> > 
> > Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
> 
> Hi all,
> 
> I am also currently working on MST emulation for VKMS. If someone can read 
> what I already did and at tell me if my implementation seems on the right 
> track it could be nice.
> 
> The current status is not very advanced: I can emulate a mst HUB, but not 
> a screen. I am currently working on properly emulating the HUB by using an 
> other hub.
> 
> You can find the branch for this work here:
> https://gitlab.freedesktop.org/louischauvet/kernel/-/tree/b4/vkms-mst

I think this is exactly the kind of things where we'll want eBPF I
think. There's no way you'll be able to model each possible test
scenarios for MST through configfs, even more so with a stable
interface.

Maxime

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

Re: [PATCH RFC v2 00/16] drm/vkms: ConfigFS interface

[Louis Chauvet]

Hi,

> > Hi all,
> > 
> > I am also currently working on MST emulation for VKMS. If someone can read 
> > what I already did and at tell me if my implementation seems on the right 
> > track it could be nice.
> > 
> > The current status is not very advanced: I can emulate a mst HUB, but not 
> > a screen. I am currently working on properly emulating the HUB by using an 
> > other hub.
> > 
> > You can find the branch for this work here:
> > https://gitlab.freedesktop.org/louischauvet/kernel/-/tree/b4/vkms-mst
> 
> I think this is exactly the kind of things where we'll want eBPF I
> think. There's no way you'll be able to model each possible test
> scenarios for MST through configfs, even more so with a stable
> interface.

I just spent some time to think about it. I agree that eBPF seems 
applicable: we want to allows userspace to emulate any MST device, and we 
don't want to create huge uAPI + whole emulation in the kernel.

As most of the protocol is similar accros device kind, I currently built 
my code around the struct vkms_mst_sideband_helpers to specify only the 
changed behavior (this way the "write to registers" "parse command"... is 
only done in one place). The choice of function is not definitive at all 
(it was just practical at this moment).

With eBPF, I know three different way to attach programs to the kernel:
 - Using a kprobe/attaching to a function: I can change the behavior of 
   all the device, there is no way "attach prog1 to hub" and "attach prog2 
   to screen1", it will be "attach prog1 to the function 
   `vkms_mst_emulator_handle_transfer_write`, and all the device will be 
   affected. This should be very easy to implement (maybe it already 
   works without modification?), but very limited / make userspace stuff 
   very ugly => Not ideal at all
 - Creating a whole architecture to attach eBPF programs in vkms: VKMS 
   manage the list of attached eBPF programs, call them when it needs... 
   This is probably the "most flexible" option (in the sense "VKMS can do 
   whatever we need to fit our usecase"). This seems not trivial to 
   implement (drm complexity + MST complexity + BPF complexity in the same 
   driver seems a bad idea, espicially because VKMS don't have a lot of 
   maintainance and I don't feel confident introducing more complexity)
   => Can work, require some work, but may bring more complexity in VKMS
 - Using BPF struct_ops: I can "simply" create/complete a struct ops for 
   diverse mst callbacks (see the proposition bellow). I looked at some 
   example, this seems to be "easy" to do, and the work in VKMS should be 
   limited.
   => Can work, a bit less flexible than the previous solution, but avoid 
   a lot of complexity

I don't know if I will be able to finish the implementation but I imagine 
something like that may be a nice interface (may be not possible "as is"):

// vkms_mst.c struct_ops that can be filled by userspace with custom 
// functions
// Known limitation: maximum 64 functions in this table
struct vkms_mst_ops {
	// Completly overide the transfer function, so the userspace can 
	// do whatever he wants (even emulating a complex MST tree 
	// without using stuff in vkms)
	handle_transfer(drm_dp_aux_msg); 

	// If default transfer function is used, those are the callback 
	// you can use (again, they will come with default 
	// implementation)
	clear_payload_id_table(..);
	link_address(..);
	enum_path_ressources(..);
	[...]

	// Used to identify this kind of device, in my example the 
	// userspace could register "LG_screen", "dell dock", "HP screen", 
	// and then configure each mst device with the correct kind
	name = "<unique name for this device kind>",

	// If you want to use the default "hub" implementation, but only 
	// tweak one specific behavior, you can use this
	base = "<name of the other structops>"
}


// Needed to implement eBPF struct_ops, called when userspace registers a
// struct_ops of type vkms_mst_ops
void register_struct_ops(new_ops...) {
	vkms_registered_ops.append(new_ops).
}

// In vkms_connector.c
// Callback called by drm core to do transfer on the connector
void vkms_mst_transfer(aux, msg) {
	mst_emulator = get_mst_emulator(aux);

	ops = vkms_registered_ops.search_for(mst_emulator.name);
	ops->handle_transfer(msg);
}

// mst_ebpf.c In the BPF program, userspace side
void handle_transfer(...) {
	[...]
}
struct vkms_mst_ops {
	handle_transfer = handle_transfer;
	name = "lg-screen";
	base = "default-screen"
}

How to use it (screen directly connected to connector, or complete
emulation by the eBPF program):

	gcc mst_ebpf.c
	bpftool register structops mst_ebpf
	# Create vkms device + connector
	mkdir -p /configfs/vkms/mydev/connectors/connector1
	#[skipped initialization of plane/crtc...]
	mkdir -p /configfs/vkms/mydev/mst_devices/device1
	echo "lg-screen" > /configfs/vkms/mydev/mst_devices/device1/name
	ln -s /configfs/vkms/mydev/connectors/connector1/device /configfs/vkms/mydev/mst_devices/device1

How to use it (hub + two screens, using vkms scaffolding to make the
emulation easier) (the idea is to do something like the tcp_congestion 
algorithm, where a few of them are implemented in the kernel, but 
userspace can inject custom implementations):

	bpftool register mst_ebpf_screen1 # struct_ops with name=lg-screen
	bpftool register mst_ebpf_screen2 # struct_ops with name=hp-screen
	#[skiped initialization of vkms dev]
	mkdir -p /configfs/vkms/mydev/mst_devices/screen1
	mkdir -p /configfs/vkms/mydev/mst_devices/screen2
	mkdir -p /configfs/vkms/mydev/mst_devices/hub
	echo "lg-screen" > /configfs/vkms/mydev/mst_devices/screen1/name
	echo "hp-screen" > /configfs/vkms/mydev/mst_devices/screen2/name
	# default-hub is the default hub emulation provided by VKMS
	echo "default-hub" > configfs/vkms/mydev/mst_devices/hub/name
	ln -s /configfs/vkms/mydev/connectors/connector1/device /configfs/vkms/mydev/mst_devices/hub
	ln -s /configfs/vkms/mydev/mst_devices/hub/child1 /configfs/vkms/mydev/mst_devices/screen1
	ln -s /configfs/vkms/mydev/mst_devices/hub/child2 /configfs/vkms/mydev/mst_devices/screen2

A few things that this approach can bring:
 - Full emulation by userspace (just add one device and provide an eBPF
   program that emulates the whole MST chain)
 - Partial emulation of devices (e.g., the default-screen implementation is
   sufficient, but you want to tweak something inside)
 - Full emulation by the kernel, using default implementations (I think
   only hub and screen, just to be able to emulate the "basic"
   configurations)
 - And cool new to reduce the "it should be perfect from the start", if we 
   use kfunc + struct_ops, both can change a little bit over time (kfunc 
   are not part of uAPI and struct_ops allows to add argument/functions). 
   Stabilisation can come later.

What do you think about this idea ?

My current plan is to continue on the "kernel-only" implementation, so I 
can have a working poc, and then switch to the eBPF work after.

Thanks,
Louis Chauvet

> Maxime

Re: [PATCH RFC v2 00/16] drm/vkms: ConfigFS interface

[Maxime Ripard]

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

On Tue, Dec 17, 2024 at 05:42:56PM +0100, Louis Chauvet wrote:
> Hi,
> 
> > > Hi all,
> > > 
> > > I am also currently working on MST emulation for VKMS. If someone can read 
> > > what I already did and at tell me if my implementation seems on the right 
> > > track it could be nice.
> > > 
> > > The current status is not very advanced: I can emulate a mst HUB, but not 
> > > a screen. I am currently working on properly emulating the HUB by using an 
> > > other hub.
> > > 
> > > You can find the branch for this work here:
> > > https://gitlab.freedesktop.org/louischauvet/kernel/-/tree/b4/vkms-mst
> > 
> > I think this is exactly the kind of things where we'll want eBPF I
> > think. There's no way you'll be able to model each possible test
> > scenarios for MST through configfs, even more so with a stable
> > interface.
> 
> I just spent some time to think about it. I agree that eBPF seems 
> applicable: we want to allows userspace to emulate any MST device, and we 
> don't want to create huge uAPI + whole emulation in the kernel.
> 
> As most of the protocol is similar accros device kind, I currently built 
> my code around the struct vkms_mst_sideband_helpers to specify only the 
> changed behavior (this way the "write to registers" "parse command"... is 
> only done in one place). The choice of function is not definitive at all 
> (it was just practical at this moment).
> 
> With eBPF, I know three different way to attach programs to the kernel:
>  - Using a kprobe/attaching to a function: I can change the behavior of 
>    all the device, there is no way "attach prog1 to hub" and "attach prog2 
>    to screen1", it will be "attach prog1 to the function 
>    `vkms_mst_emulator_handle_transfer_write`, and all the device will be 
>    affected. This should be very easy to implement (maybe it already 
>    works without modification?), but very limited / make userspace stuff 
>    very ugly => Not ideal at all
>  - Creating a whole architecture to attach eBPF programs in vkms: VKMS 
>    manage the list of attached eBPF programs, call them when it needs... 
>    This is probably the "most flexible" option (in the sense "VKMS can do 
>    whatever we need to fit our usecase"). This seems not trivial to 
>    implement (drm complexity + MST complexity + BPF complexity in the same 
>    driver seems a bad idea, espicially because VKMS don't have a lot of 
>    maintainance and I don't feel confident introducing more complexity)
>    => Can work, require some work, but may bring more complexity in VKMS
>  - Using BPF struct_ops: I can "simply" create/complete a struct ops for 
>    diverse mst callbacks (see the proposition bellow). I looked at some 
>    example, this seems to be "easy" to do, and the work in VKMS should be 
>    limited.
>    => Can work, a bit less flexible than the previous solution, but avoid 
>    a lot of complexity
> 
> I don't know if I will be able to finish the implementation but I imagine 
> something like that may be a nice interface (may be not possible "as is"):
> 
> // vkms_mst.c struct_ops that can be filled by userspace with custom 
> // functions
> // Known limitation: maximum 64 functions in this table
> struct vkms_mst_ops {
> 	// Completly overide the transfer function, so the userspace can 
> 	// do whatever he wants (even emulating a complex MST tree 
> 	// without using stuff in vkms)
> 	handle_transfer(drm_dp_aux_msg); 
> 
> 	// If default transfer function is used, those are the callback 
> 	// you can use (again, they will come with default 
> 	// implementation)
> 	clear_payload_id_table(..);
> 	link_address(..);
> 	enum_path_ressources(..);
> 	[...]
> 
> 	// Used to identify this kind of device, in my example the 
> 	// userspace could register "LG_screen", "dell dock", "HP screen", 
> 	// and then configure each mst device with the correct kind
> 	name = "<unique name for this device kind>",
> 
> 	// If you want to use the default "hub" implementation, but only 
> 	// tweak one specific behavior, you can use this
> 	base = "<name of the other structops>"
> }
> 
> 
> // Needed to implement eBPF struct_ops, called when userspace registers a
> // struct_ops of type vkms_mst_ops
> void register_struct_ops(new_ops...) {
> 	vkms_registered_ops.append(new_ops).
> }
> 
> // In vkms_connector.c
> // Callback called by drm core to do transfer on the connector
> void vkms_mst_transfer(aux, msg) {
> 	mst_emulator = get_mst_emulator(aux);
> 
> 	ops = vkms_registered_ops.search_for(mst_emulator.name);
> 	ops->handle_transfer(msg);
> }
> 
> // mst_ebpf.c In the BPF program, userspace side
> void handle_transfer(...) {
> 	[...]
> }
> struct vkms_mst_ops {
> 	handle_transfer = handle_transfer;
> 	name = "lg-screen";
> 	base = "default-screen"
> }

I don't think MST is the right abstraction there. We should have MST
related helper functions available to eBPF programs, but we should load
them at the connector level, ie, implement a full blown connector
atomic_check for example. It's more flexible and will allow to implement
other use-cases people have been interested in (like panels).

Maxime

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

Re: [PATCH RFC v2 00/16] drm/vkms: ConfigFS interface

[Simona Vetter]

On Wed, Feb 19, 2025 at 10:28:56AM +0100, Maxime Ripard wrote:
> On Tue, Dec 17, 2024 at 05:42:56PM +0100, Louis Chauvet wrote:
> > Hi,
> > 
> > > > Hi all,
> > > > 
> > > > I am also currently working on MST emulation for VKMS. If someone can read 
> > > > what I already did and at tell me if my implementation seems on the right 
> > > > track it could be nice.
> > > > 
> > > > The current status is not very advanced: I can emulate a mst HUB, but not 
> > > > a screen. I am currently working on properly emulating the HUB by using an 
> > > > other hub.
> > > > 
> > > > You can find the branch for this work here:
> > > > https://gitlab.freedesktop.org/louischauvet/kernel/-/tree/b4/vkms-mst

Can't look at this because it's private.

> > > I think this is exactly the kind of things where we'll want eBPF I
> > > think. There's no way you'll be able to model each possible test
> > > scenarios for MST through configfs, even more so with a stable
> > > interface.
> > 
> > I just spent some time to think about it. I agree that eBPF seems 
> > applicable: we want to allows userspace to emulate any MST device, and we 
> > don't want to create huge uAPI + whole emulation in the kernel.
> > 
> > As most of the protocol is similar accros device kind, I currently built 
> > my code around the struct vkms_mst_sideband_helpers to specify only the 
> > changed behavior (this way the "write to registers" "parse command"... is 
> > only done in one place). The choice of function is not definitive at all 
> > (it was just practical at this moment).
> > 
> > With eBPF, I know three different way to attach programs to the kernel:
> >  - Using a kprobe/attaching to a function: I can change the behavior of 
> >    all the device, there is no way "attach prog1 to hub" and "attach prog2 
> >    to screen1", it will be "attach prog1 to the function 
> >    `vkms_mst_emulator_handle_transfer_write`, and all the device will be 
> >    affected. This should be very easy to implement (maybe it already 
> >    works without modification?), but very limited / make userspace stuff 
> >    very ugly => Not ideal at all
> >  - Creating a whole architecture to attach eBPF programs in vkms: VKMS 
> >    manage the list of attached eBPF programs, call them when it needs... 
> >    This is probably the "most flexible" option (in the sense "VKMS can do 
> >    whatever we need to fit our usecase"). This seems not trivial to 
> >    implement (drm complexity + MST complexity + BPF complexity in the same 
> >    driver seems a bad idea, espicially because VKMS don't have a lot of 
> >    maintainance and I don't feel confident introducing more complexity)
> >    => Can work, require some work, but may bring more complexity in VKMS
> >  - Using BPF struct_ops: I can "simply" create/complete a struct ops for 
> >    diverse mst callbacks (see the proposition bellow). I looked at some 
> >    example, this seems to be "easy" to do, and the work in VKMS should be 
> >    limited.
> >    => Can work, a bit less flexible than the previous solution, but avoid 
> >    a lot of complexity
> > 
> > I don't know if I will be able to finish the implementation but I imagine 
> > something like that may be a nice interface (may be not possible "as is"):
> > 
> > // vkms_mst.c struct_ops that can be filled by userspace with custom 
> > // functions
> > // Known limitation: maximum 64 functions in this table
> > struct vkms_mst_ops {
> > 	// Completly overide the transfer function, so the userspace can 
> > 	// do whatever he wants (even emulating a complex MST tree 
> > 	// without using stuff in vkms)
> > 	handle_transfer(drm_dp_aux_msg); 
> > 
> > 	// If default transfer function is used, those are the callback 
> > 	// you can use (again, they will come with default 
> > 	// implementation)
> > 	clear_payload_id_table(..);
> > 	link_address(..);
> > 	enum_path_ressources(..);
> > 	[...]
> > 
> > 	// Used to identify this kind of device, in my example the 
> > 	// userspace could register "LG_screen", "dell dock", "HP screen", 
> > 	// and then configure each mst device with the correct kind
> > 	name = "<unique name for this device kind>",
> > 
> > 	// If you want to use the default "hub" implementation, but only 
> > 	// tweak one specific behavior, you can use this
> > 	base = "<name of the other structops>"
> > }
> > 
> > 
> > // Needed to implement eBPF struct_ops, called when userspace registers a
> > // struct_ops of type vkms_mst_ops
> > void register_struct_ops(new_ops...) {
> > 	vkms_registered_ops.append(new_ops).
> > }
> > 
> > // In vkms_connector.c
> > // Callback called by drm core to do transfer on the connector
> > void vkms_mst_transfer(aux, msg) {
> > 	mst_emulator = get_mst_emulator(aux);
> > 
> > 	ops = vkms_registered_ops.search_for(mst_emulator.name);
> > 	ops->handle_transfer(msg);
> > }
> > 
> > // mst_ebpf.c In the BPF program, userspace side
> > void handle_transfer(...) {
> > 	[...]
> > }
> > struct vkms_mst_ops {
> > 	handle_transfer = handle_transfer;
> > 	name = "lg-screen";
> > 	base = "default-screen"
> > }
> 
> I don't think MST is the right abstraction there. We should have MST
> related helper functions available to eBPF programs, but we should load
> them at the connector level, ie, implement a full blown connector
> atomic_check for example. It's more flexible and will allow to implement
> other use-cases people have been interested in (like panels).

So since I can't look at the code I'll just drop my thoughts here.

- I think validating the MST helpers implementation should be done with
  kunit tests. So these are out of scope for vkms testing I think
  entirely.

- Next up are the driver implementations. There we might want to be able
  to inject fake mst devices to stress-test driver corner cases. But I
  don't think that's a job for vkms either.

- Now for vkms itself, I think the interesting case here is being able to
  test compositors against funny mst corner-cases, but for that we don't
  really need an mst operation at all. So no dp-aux or anything like that,
  we just hotplug-create connectors with names and PATH property and MST
  type, without any of the kernel-internal presentations for hubs/branch
  points and all that stuff. Because userspace doesn't ever see that.

- Next up is expressing all the funny constraints that can result in,
  across different drivers. For that I think we want ebpf to implement the
  various atomic_check hooks, so that you can implement all the funny
  constraints of mst link bw limitations, but also host-side display
  limitations. And I concur with Maxime that this ebpf support should be
  entirely agnostic and just allow you to attach atomic_check
  implementations to connectors, planes and crtcs. And then maybe one for
  the overall commit, so that global constraints are easier to implement.

So summary: MST testing in vkms only needs to look like MST to userspace.
Internally we'll just hand-roll the entire connector hotplug and leave out
everything else. Because testing driver dp mst implementations and the
helpers is better covered through different means.

Eventually we might want to implement fake i2c and dp-aux implementations
for additional features like TV remote control and fun stuff like that (I
forgot the CEA/HDMI name for these). But I think that's waaaayyyyyy down
the road.

Cheers, Sima
-- 
Simona Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH RFC v2 00/16] drm/vkms: ConfigFS interface

[Louis Chauvet]

Le 19/02/2025 à 14:15, Simona Vetter a écrit :
> On Wed, Feb 19, 2025 at 10:28:56AM +0100, Maxime Ripard wrote:
>> On Tue, Dec 17, 2024 at 05:42:56PM +0100, Louis Chauvet wrote:
>>> Hi,
>>>
>>>>> Hi all,
>>>>>
>>>>> I am also currently working on MST emulation for VKMS. If someone can read
>>>>> what I already did and at tell me if my implementation seems on the right
>>>>> track it could be nice.
>>>>>
>>>>> The current status is not very advanced: I can emulate a mst HUB, but not
>>>>> a screen. I am currently working on properly emulating the HUB by using an
>>>>> other hub.
>>>>>
>>>>> You can find the branch for this work here:
>>>>> https://gitlab.freedesktop.org/louischauvet/kernel/-/tree/b4/vkms-mst
> 
> Can't look at this because it's private.

Hi Maxime, Sima,

Normally, those should work:

https://gitlab.freedesktop.org/louischauvet/kernel/-/tree/vkms-mst
https://github.com/Fomys/linux/tree/vkms-mst

I just re-tested, this is broken, probably because I never had the time 
to properly finish my last idea: simplifying vkms_connector by creating 
vkms_mst_emulator_root. With the rest of the code (i.e. 
vkms_mst_hub/display_emulator + vkms_connector), I was able to make this 
config working:

HUB1 -> HUB2 -> DISPLAY1
      |       -> DISPLAY2
      -> DISPLAY3

(working == it was listed properly by modetest + did not report any 
issue when using a connector with modetest -s)

Few things to note: no ConfigFS support, no eBPF support, poorly tested 
(there are probably multithread/recursion issues)

I had to stop working on it because I don't have anymore time, I plan to 
at least rebase + send an RFC once the VKMS+ConfigFS work is done.

>>>> I think this is exactly the kind of things where we'll want eBPF I
>>>> think. There's no way you'll be able to model each possible test
>>>> scenarios for MST through configfs, even more so with a stable
>>>> interface.
>>>
>>> I just spent some time to think about it. I agree that eBPF seems
>>> applicable: we want to allows userspace to emulate any MST device, and we
>>> don't want to create huge uAPI + whole emulation in the kernel.
>>>
>>> As most of the protocol is similar accros device kind, I currently built
>>> my code around the struct vkms_mst_sideband_helpers to specify only the
>>> changed behavior (this way the "write to registers" "parse command"... is
>>> only done in one place). The choice of function is not definitive at all
>>> (it was just practical at this moment).
>>>
>>> With eBPF, I know three different way to attach programs to the kernel:
>>>   - Using a kprobe/attaching to a function: I can change the behavior of
>>>     all the device, there is no way "attach prog1 to hub" and "attach prog2
>>>     to screen1", it will be "attach prog1 to the function
>>>     `vkms_mst_emulator_handle_transfer_write`, and all the device will be
>>>     affected. This should be very easy to implement (maybe it already
>>>     works without modification?), but very limited / make userspace stuff
>>>     very ugly => Not ideal at all
>>>   - Creating a whole architecture to attach eBPF programs in vkms: VKMS
>>>     manage the list of attached eBPF programs, call them when it needs...
>>>     This is probably the "most flexible" option (in the sense "VKMS can do
>>>     whatever we need to fit our usecase"). This seems not trivial to
>>>     implement (drm complexity + MST complexity + BPF complexity in the same
>>>     driver seems a bad idea, espicially because VKMS don't have a lot of
>>>     maintainance and I don't feel confident introducing more complexity)
>>>     => Can work, require some work, but may bring more complexity in VKMS
>>>   - Using BPF struct_ops: I can "simply" create/complete a struct ops for
>>>     diverse mst callbacks (see the proposition bellow). I looked at some
>>>     example, this seems to be "easy" to do, and the work in VKMS should be
>>>     limited.
>>>     => Can work, a bit less flexible than the previous solution, but avoid
>>>     a lot of complexity
>>>
>>> I don't know if I will be able to finish the implementation but I imagine
>>> something like that may be a nice interface (may be not possible "as is"):
>>>
>>> // vkms_mst.c struct_ops that can be filled by userspace with custom
>>> // functions
>>> // Known limitation: maximum 64 functions in this table
>>> struct vkms_mst_ops {
>>> 	// Completly overide the transfer function, so the userspace can
>>> 	// do whatever he wants (even emulating a complex MST tree
>>> 	// without using stuff in vkms)
>>> 	handle_transfer(drm_dp_aux_msg);
>>>
>>> 	// If default transfer function is used, those are the callback
>>> 	// you can use (again, they will come with default
>>> 	// implementation)
>>> 	clear_payload_id_table(..);
>>> 	link_address(..);
>>> 	enum_path_ressources(..);
>>> 	[...]
>>>
>>> 	// Used to identify this kind of device, in my example the
>>> 	// userspace could register "LG_screen", "dell dock", "HP screen",
>>> 	// and then configure each mst device with the correct kind
>>> 	name = "<unique name for this device kind>",
>>>
>>> 	// If you want to use the default "hub" implementation, but only
>>> 	// tweak one specific behavior, you can use this
>>> 	base = "<name of the other structops>"
>>> }
>>>
>>>
>>> // Needed to implement eBPF struct_ops, called when userspace registers a
>>> // struct_ops of type vkms_mst_ops
>>> void register_struct_ops(new_ops...) {
>>> 	vkms_registered_ops.append(new_ops).
>>> }
>>>
>>> // In vkms_connector.c
>>> // Callback called by drm core to do transfer on the connector
>>> void vkms_mst_transfer(aux, msg) {
>>> 	mst_emulator = get_mst_emulator(aux);
>>>
>>> 	ops = vkms_registered_ops.search_for(mst_emulator.name);
>>> 	ops->handle_transfer(msg);
>>> }
>>>
>>> // mst_ebpf.c In the BPF program, userspace side
>>> void handle_transfer(...) {
>>> 	[...]
>>> }
>>> struct vkms_mst_ops {
>>> 	handle_transfer = handle_transfer;
>>> 	name = "lg-screen";
>>> 	base = "default-screen"
>>> }
>>
>> I don't think MST is the right abstraction there. We should have MST
>> related helper functions available to eBPF programs, but we should load
>> them at the connector level, ie, implement a full blown connector
>> atomic_check for example. It's more flexible and will allow to implement
>> other use-cases people have been interested in (like panels).
> 
> So since I can't look at the code I'll just drop my thoughts here.
> 
> - I think validating the MST helpers implementation should be done with
>    kunit tests. So these are out of scope for vkms testing I think
>    entirely.

Yes, I agree with this, and it joins your last comment: the full dp-aux 
emulation does not belong only to VKMS. I had this idea only because my 
solution only use the normal core MST implementation (no special 
handling in VKMS, just pure dp-aux emulation), so technically you could 
also stress-test drm core with it.

> - Next up are the driver implementations. There we might want to be able
>    to inject fake mst devices to stress-test driver corner cases. But I
>    don't think that's a job for vkms either.

I agree, VKMS is not here to test other drivers.

> - Now for vkms itself, I think the interesting case here is being able to
>    test compositors against funny mst corner-cases, but for that we don't
>    really need an mst operation at all. So no dp-aux or anything like that,
>    we just hotplug-create connectors with names and PATH property and MST
>    type, without any of the kernel-internal presentations for hubs/branch
>    points and all that stuff. Because userspace doesn't ever see that.

I knew that user space don't really see the MST information (apart from 
PATH), but I did not think about this solution. This may work well to 
test user space, I agree!

I think we are on the good track with José, he is trying to implement 
connector hot-creation through ConfigFS [1]. To add "MST emulation", we 
can "simply" add the PATH property through ConfigFS.

[1]: Few discussions here 
https://lore.kernel.org/all/Z5zJ1rEZyBEgd7DN@louis-chauvet-laptop/

> - Next up is expressing all the funny constraints that can result in,
>    across different drivers. For that I think we want ebpf to implement the
>    various atomic_check hooks, so that you can implement all the funny
>    constraints of mst link bw limitations, but also host-side display
>    limitations. And I concur with Maxime that this ebpf support should be
>    entirely agnostic and just allow you to attach atomic_check
>    implementations to connectors, planes and crtcs. And then maybe one for
>    the overall commit, so that global constraints are easier to implement.

If I understand correctly, this has nothing to do with VKMS + MST?
I don't clearly understand the use case: why do we want the user space 
to express hardware limitations? If you have already discussed this on 
the mailing list, can you share the discussion?

> So summary: MST testing in vkms only needs to look like MST to userspace.
> Internally we'll just hand-roll the entire connector hotplug and leave out
> everything else. Because testing driver dp mst implementations and the
> helpers is better covered through different means.
 >
> Eventually we might want to implement fake i2c and dp-aux implementations
> for additional features like TV remote control and fun stuff like that (I
> forgot the CEA/HDMI name for these). But I think that's waaaayyyyyy down
> the road.

I think I am not that far from a full dp-aux emulation, it is precisely 
what I tried to do in VKMS. I don't have the time to transform it to 
kunit tests.

Thanks a lot for your feedback!
Louis Chauvet

> Cheers, Sima

-- 
Louis Chauvet, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH RFC v2 00/16] drm/vkms: ConfigFS interface

[Simona Vetter]

On Wed, Feb 19, 2025 at 05:28:37PM +0100, Louis Chauvet wrote:
> Le 19/02/2025 à 14:15, Simona Vetter a écrit :
> > On Wed, Feb 19, 2025 at 10:28:56AM +0100, Maxime Ripard wrote:
> > > On Tue, Dec 17, 2024 at 05:42:56PM +0100, Louis Chauvet wrote:
> > > > Hi,
> > > > 
> > > > > > Hi all,
> > > > > > 
> > > > > > I am also currently working on MST emulation for VKMS. If someone can read
> > > > > > what I already did and at tell me if my implementation seems on the right
> > > > > > track it could be nice.
> > > > > > 
> > > > > > The current status is not very advanced: I can emulate a mst HUB, but not
> > > > > > a screen. I am currently working on properly emulating the HUB by using an
> > > > > > other hub.
> > > > > > 
> > > > > > You can find the branch for this work here:
> > > > > > https://gitlab.freedesktop.org/louischauvet/kernel/-/tree/b4/vkms-mst
> > 
> > Can't look at this because it's private.
> 
> Hi Maxime, Sima,
> 
> Normally, those should work:
> 
> https://gitlab.freedesktop.org/louischauvet/kernel/-/tree/vkms-mst
> https://github.com/Fomys/linux/tree/vkms-mst
> 
> I just re-tested, this is broken, probably because I never had the time to
> properly finish my last idea: simplifying vkms_connector by creating
> vkms_mst_emulator_root. With the rest of the code (i.e.
> vkms_mst_hub/display_emulator + vkms_connector), I was able to make this
> config working:
> 
> HUB1 -> HUB2 -> DISPLAY1
>      |       -> DISPLAY2
>      -> DISPLAY3
> 
> (working == it was listed properly by modetest + did not report any issue
> when using a connector with modetest -s)
> 
> Few things to note: no ConfigFS support, no eBPF support, poorly tested
> (there are probably multithread/recursion issues)
> 
> I had to stop working on it because I don't have anymore time, I plan to at
> least rebase + send an RFC once the VKMS+ConfigFS work is done.
> 
> > > > > I think this is exactly the kind of things where we'll want eBPF I
> > > > > think. There's no way you'll be able to model each possible test
> > > > > scenarios for MST through configfs, even more so with a stable
> > > > > interface.
> > > > 
> > > > I just spent some time to think about it. I agree that eBPF seems
> > > > applicable: we want to allows userspace to emulate any MST device, and we
> > > > don't want to create huge uAPI + whole emulation in the kernel.
> > > > 
> > > > As most of the protocol is similar accros device kind, I currently built
> > > > my code around the struct vkms_mst_sideband_helpers to specify only the
> > > > changed behavior (this way the "write to registers" "parse command"... is
> > > > only done in one place). The choice of function is not definitive at all
> > > > (it was just practical at this moment).
> > > > 
> > > > With eBPF, I know three different way to attach programs to the kernel:
> > > >   - Using a kprobe/attaching to a function: I can change the behavior of
> > > >     all the device, there is no way "attach prog1 to hub" and "attach prog2
> > > >     to screen1", it will be "attach prog1 to the function
> > > >     `vkms_mst_emulator_handle_transfer_write`, and all the device will be
> > > >     affected. This should be very easy to implement (maybe it already
> > > >     works without modification?), but very limited / make userspace stuff
> > > >     very ugly => Not ideal at all
> > > >   - Creating a whole architecture to attach eBPF programs in vkms: VKMS
> > > >     manage the list of attached eBPF programs, call them when it needs...
> > > >     This is probably the "most flexible" option (in the sense "VKMS can do
> > > >     whatever we need to fit our usecase"). This seems not trivial to
> > > >     implement (drm complexity + MST complexity + BPF complexity in the same
> > > >     driver seems a bad idea, espicially because VKMS don't have a lot of
> > > >     maintainance and I don't feel confident introducing more complexity)
> > > >     => Can work, require some work, but may bring more complexity in VKMS
> > > >   - Using BPF struct_ops: I can "simply" create/complete a struct ops for
> > > >     diverse mst callbacks (see the proposition bellow). I looked at some
> > > >     example, this seems to be "easy" to do, and the work in VKMS should be
> > > >     limited.
> > > >     => Can work, a bit less flexible than the previous solution, but avoid
> > > >     a lot of complexity
> > > > 
> > > > I don't know if I will be able to finish the implementation but I imagine
> > > > something like that may be a nice interface (may be not possible "as is"):
> > > > 
> > > > // vkms_mst.c struct_ops that can be filled by userspace with custom
> > > > // functions
> > > > // Known limitation: maximum 64 functions in this table
> > > > struct vkms_mst_ops {
> > > > 	// Completly overide the transfer function, so the userspace can
> > > > 	// do whatever he wants (even emulating a complex MST tree
> > > > 	// without using stuff in vkms)
> > > > 	handle_transfer(drm_dp_aux_msg);
> > > > 
> > > > 	// If default transfer function is used, those are the callback
> > > > 	// you can use (again, they will come with default
> > > > 	// implementation)
> > > > 	clear_payload_id_table(..);
> > > > 	link_address(..);
> > > > 	enum_path_ressources(..);
> > > > 	[...]
> > > > 
> > > > 	// Used to identify this kind of device, in my example the
> > > > 	// userspace could register "LG_screen", "dell dock", "HP screen",
> > > > 	// and then configure each mst device with the correct kind
> > > > 	name = "<unique name for this device kind>",
> > > > 
> > > > 	// If you want to use the default "hub" implementation, but only
> > > > 	// tweak one specific behavior, you can use this
> > > > 	base = "<name of the other structops>"
> > > > }
> > > > 
> > > > 
> > > > // Needed to implement eBPF struct_ops, called when userspace registers a
> > > > // struct_ops of type vkms_mst_ops
> > > > void register_struct_ops(new_ops...) {
> > > > 	vkms_registered_ops.append(new_ops).
> > > > }
> > > > 
> > > > // In vkms_connector.c
> > > > // Callback called by drm core to do transfer on the connector
> > > > void vkms_mst_transfer(aux, msg) {
> > > > 	mst_emulator = get_mst_emulator(aux);
> > > > 
> > > > 	ops = vkms_registered_ops.search_for(mst_emulator.name);
> > > > 	ops->handle_transfer(msg);
> > > > }
> > > > 
> > > > // mst_ebpf.c In the BPF program, userspace side
> > > > void handle_transfer(...) {
> > > > 	[...]
> > > > }
> > > > struct vkms_mst_ops {
> > > > 	handle_transfer = handle_transfer;
> > > > 	name = "lg-screen";
> > > > 	base = "default-screen"
> > > > }
> > > 
> > > I don't think MST is the right abstraction there. We should have MST
> > > related helper functions available to eBPF programs, but we should load
> > > them at the connector level, ie, implement a full blown connector
> > > atomic_check for example. It's more flexible and will allow to implement
> > > other use-cases people have been interested in (like panels).
> > 
> > So since I can't look at the code I'll just drop my thoughts here.
> > 
> > - I think validating the MST helpers implementation should be done with
> >    kunit tests. So these are out of scope for vkms testing I think
> >    entirely.
> 
> Yes, I agree with this, and it joins your last comment: the full dp-aux
> emulation does not belong only to VKMS. I had this idea only because my
> solution only use the normal core MST implementation (no special handling in
> VKMS, just pure dp-aux emulation), so technically you could also stress-test
> drm core with it.
> 
> > - Next up are the driver implementations. There we might want to be able
> >    to inject fake mst devices to stress-test driver corner cases. But I
> >    don't think that's a job for vkms either.
> 
> I agree, VKMS is not here to test other drivers.
> 
> > - Now for vkms itself, I think the interesting case here is being able to
> >    test compositors against funny mst corner-cases, but for that we don't
> >    really need an mst operation at all. So no dp-aux or anything like that,
> >    we just hotplug-create connectors with names and PATH property and MST
> >    type, without any of the kernel-internal presentations for hubs/branch
> >    points and all that stuff. Because userspace doesn't ever see that.
> 
> I knew that user space don't really see the MST information (apart from
> PATH), but I did not think about this solution. This may work well to test
> user space, I agree!
> 
> I think we are on the good track with José, he is trying to implement
> connector hot-creation through ConfigFS [1]. To add "MST emulation", we can
> "simply" add the PATH property through ConfigFS.

Yeah, I think that's the way to go. Well maybe with a change to always
include the PATH property, because currently that's true for all
hotpluggable connectors. And we probably want to keep that if we extend it
to hotpluggable bridges or similar.

> [1]: Few discussions here
> https://lore.kernel.org/all/Z5zJ1rEZyBEgd7DN@louis-chauvet-laptop/

Agreeing with you, connector hotplug is something we need to tackle as an
extension of the basic configfs support, since it's quite complex.

> > - Next up is expressing all the funny constraints that can result in,
> >    across different drivers. For that I think we want ebpf to implement the
> >    various atomic_check hooks, so that you can implement all the funny
> >    constraints of mst link bw limitations, but also host-side display
> >    limitations. And I concur with Maxime that this ebpf support should be
> >    entirely agnostic and just allow you to attach atomic_check
> >    implementations to connectors, planes and crtcs. And then maybe one for
> >    the overall commit, so that global constraints are easier to implement.
> 
> If I understand correctly, this has nothing to do with VKMS + MST?
> I don't clearly understand the use case: why do we want the user space to
> express hardware limitations? If you have already discussed this on the
> mailing list, can you share the discussion?

Not sure this was discussed in-depth, but when you get into more complex
output configurations, there's all kinds of funny hw constraints that pop
up. Examples:
- Multiple mst outputs on the same physical port share the overall
  bandwidth. So individually you might be able to light up each connector
  at max resolution, but if you try to light up all of them, there's a
  limitation. This is the mst specific case.
- There's also lots of display hw constraints, like a limited amount of
  clocks (fewer than crtc), or memory bw constraints for scanout, and
  similar things.

The idea is to express these constraints with ebpf programs, so that you
can validate that a compositor correctly handles these cases and doesn't
try an invalide configuration and then just fails instead of trying to
fall back to something that works.

So it's a much bigger issue, but multi-connector mst is a fairly important
case here.

Cheers, Sima

> > So summary: MST testing in vkms only needs to look like MST to userspace.
> > Internally we'll just hand-roll the entire connector hotplug and leave out
> > everything else. Because testing driver dp mst implementations and the
> > helpers is better covered through different means.
> >
> > Eventually we might want to implement fake i2c and dp-aux implementations
> > for additional features like TV remote control and fun stuff like that (I
> > forgot the CEA/HDMI name for these). But I think that's waaaayyyyyy down
> > the road.
> 
> I think I am not that far from a full dp-aux emulation, it is precisely what
> I tried to do in VKMS. I don't have the time to transform it to kunit tests.
> 
> Thanks a lot for your feedback!
> Louis Chauvet
> 
> > Cheers, Sima
> 
> -- 
> Louis Chauvet, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
> 

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

Re: [PATCH RFC v2 00/16] drm/vkms: ConfigFS interface

[Louis Chauvet]

Le 20/02/2025 à 10:43, Simona Vetter a écrit :
> On Wed, Feb 19, 2025 at 05:28:37PM +0100, Louis Chauvet wrote:
>> Le 19/02/2025 à 14:15, Simona Vetter a écrit :
>>> On Wed, Feb 19, 2025 at 10:28:56AM +0100, Maxime Ripard wrote:
>>>> On Tue, Dec 17, 2024 at 05:42:56PM +0100, Louis Chauvet wrote:
>>>>> Hi,
>>>>>
>>>>>>> Hi all,
>>>>>>>
>>>>>>> I am also currently working on MST emulation for VKMS. If someone can read
>>>>>>> what I already did and at tell me if my implementation seems on the right
>>>>>>> track it could be nice.
>>>>>>>
>>>>>>> The current status is not very advanced: I can emulate a mst HUB, but not
>>>>>>> a screen. I am currently working on properly emulating the HUB by using an
>>>>>>> other hub.
>>>>>>>
>>>>>>> You can find the branch for this work here:
>>>>>>> https://gitlab.freedesktop.org/louischauvet/kernel/-/tree/b4/vkms-mst
>>>
>>> Can't look at this because it's private.
>>
>> Hi Maxime, Sima,
>>
>> Normally, those should work:
>>

[1]:https://gitlab.freedesktop.org/louischauvet/kernel/-/tree/vkms-mst[1]:https://github.com/Fomys/linux/tree/vkms-mst

>>
>> I just re-tested, this is broken, probably because I never had the time to
>> properly finish my last idea: simplifying vkms_connector by creating
>> vkms_mst_emulator_root. With the rest of the code (i.e.
>> vkms_mst_hub/display_emulator + vkms_connector), I was able to make this
>> config working:
>>
>> HUB1 -> HUB2 -> DISPLAY1
>>       |       -> DISPLAY2
>>       -> DISPLAY3
>>
>> (working == it was listed properly by modetest + did not report any issue
>> when using a connector with modetest -s)
>>
>> Few things to note: no ConfigFS support, no eBPF support, poorly tested
>> (there are probably multithread/recursion issues)
>>
>> I had to stop working on it because I don't have anymore time, I plan to at
>> least rebase + send an RFC once the VKMS+ConfigFS work is done.

I fixed it, so now [1] contains a working mst emulation with dp-aux 
emulation. It is far from perfect and probably have many flaws, but it 
works.

The last commit creates the following configuration:

host 0->0 hub1 1->0 display1
                2->0 hub2 1->0 display2
                          2-> Not Connected
                3-> NC
                4-> NC

>>
>>>>>> I think this is exactly the kind of things where we'll want eBPF I
>>>>>> think. There's no way you'll be able to model each possible test
>>>>>> scenarios for MST through configfs, even more so with a stable
>>>>>> interface.
>>>>>
>>>>> I just spent some time to think about it. I agree that eBPF seems
>>>>> applicable: we want to allows userspace to emulate any MST device, and we
>>>>> don't want to create huge uAPI + whole emulation in the kernel.
>>>>>
>>>>> As most of the protocol is similar accros device kind, I currently built
>>>>> my code around the struct vkms_mst_sideband_helpers to specify only the
>>>>> changed behavior (this way the "write to registers" "parse command"... is
>>>>> only done in one place). The choice of function is not definitive at all
>>>>> (it was just practical at this moment).
>>>>>
>>>>> With eBPF, I know three different way to attach programs to the kernel:
>>>>>    - Using a kprobe/attaching to a function: I can change the behavior of
>>>>>      all the device, there is no way "attach prog1 to hub" and "attach prog2
>>>>>      to screen1", it will be "attach prog1 to the function
>>>>>      `vkms_mst_emulator_handle_transfer_write`, and all the device will be
>>>>>      affected. This should be very easy to implement (maybe it already
>>>>>      works without modification?), but very limited / make userspace stuff
>>>>>      very ugly => Not ideal at all
>>>>>    - Creating a whole architecture to attach eBPF programs in vkms: VKMS
>>>>>      manage the list of attached eBPF programs, call them when it needs...
>>>>>      This is probably the "most flexible" option (in the sense "VKMS can do
>>>>>      whatever we need to fit our usecase"). This seems not trivial to
>>>>>      implement (drm complexity + MST complexity + BPF complexity in the same
>>>>>      driver seems a bad idea, espicially because VKMS don't have a lot of
>>>>>      maintainance and I don't feel confident introducing more complexity)
>>>>>      => Can work, require some work, but may bring more complexity in VKMS
>>>>>    - Using BPF struct_ops: I can "simply" create/complete a struct ops for
>>>>>      diverse mst callbacks (see the proposition bellow). I looked at some
>>>>>      example, this seems to be "easy" to do, and the work in VKMS should be
>>>>>      limited.
>>>>>      => Can work, a bit less flexible than the previous solution, but avoid
>>>>>      a lot of complexity
>>>>>
>>>>> I don't know if I will be able to finish the implementation but I imagine
>>>>> something like that may be a nice interface (may be not possible "as is"):
>>>>>
>>>>> // vkms_mst.c struct_ops that can be filled by userspace with custom
>>>>> // functions
>>>>> // Known limitation: maximum 64 functions in this table
>>>>> struct vkms_mst_ops {
>>>>> 	// Completly overide the transfer function, so the userspace can
>>>>> 	// do whatever he wants (even emulating a complex MST tree
>>>>> 	// without using stuff in vkms)
>>>>> 	handle_transfer(drm_dp_aux_msg);
>>>>>
>>>>> 	// If default transfer function is used, those are the callback
>>>>> 	// you can use (again, they will come with default
>>>>> 	// implementation)
>>>>> 	clear_payload_id_table(..);
>>>>> 	link_address(..);
>>>>> 	enum_path_ressources(..);
>>>>> 	[...]
>>>>>
>>>>> 	// Used to identify this kind of device, in my example the
>>>>> 	// userspace could register "LG_screen", "dell dock", "HP screen",
>>>>> 	// and then configure each mst device with the correct kind
>>>>> 	name = "<unique name for this device kind>",
>>>>>
>>>>> 	// If you want to use the default "hub" implementation, but only
>>>>> 	// tweak one specific behavior, you can use this
>>>>> 	base = "<name of the other structops>"
>>>>> }
>>>>>
>>>>>
>>>>> // Needed to implement eBPF struct_ops, called when userspace registers a
>>>>> // struct_ops of type vkms_mst_ops
>>>>> void register_struct_ops(new_ops...) {
>>>>> 	vkms_registered_ops.append(new_ops).
>>>>> }
>>>>>
>>>>> // In vkms_connector.c
>>>>> // Callback called by drm core to do transfer on the connector
>>>>> void vkms_mst_transfer(aux, msg) {
>>>>> 	mst_emulator = get_mst_emulator(aux);
>>>>>
>>>>> 	ops = vkms_registered_ops.search_for(mst_emulator.name);
>>>>> 	ops->handle_transfer(msg);
>>>>> }
>>>>>
>>>>> // mst_ebpf.c In the BPF program, userspace side
>>>>> void handle_transfer(...) {
>>>>> 	[...]
>>>>> }
>>>>> struct vkms_mst_ops {
>>>>> 	handle_transfer = handle_transfer;
>>>>> 	name = "lg-screen";
>>>>> 	base = "default-screen"
>>>>> }
>>>>
>>>> I don't think MST is the right abstraction there. We should have MST
>>>> related helper functions available to eBPF programs, but we should load
>>>> them at the connector level, ie, implement a full blown connector
>>>> atomic_check for example. It's more flexible and will allow to implement
>>>> other use-cases people have been interested in (like panels).
>>>
>>> So since I can't look at the code I'll just drop my thoughts here.
>>>
>>> - I think validating the MST helpers implementation should be done with
>>>     kunit tests. So these are out of scope for vkms testing I think
>>>     entirely.
>>
>> Yes, I agree with this, and it joins your last comment: the full dp-aux
>> emulation does not belong only to VKMS. I had this idea only because my
>> solution only use the normal core MST implementation (no special handling in
>> VKMS, just pure dp-aux emulation), so technically you could also stress-test
>> drm core with it.
>>
>>> - Next up are the driver implementations. There we might want to be able
>>>     to inject fake mst devices to stress-test driver corner cases. But I
>>>     don't think that's a job for vkms either.
>>
>> I agree, VKMS is not here to test other drivers.
>>
>>> - Now for vkms itself, I think the interesting case here is being able to
>>>     test compositors against funny mst corner-cases, but for that we don't
>>>     really need an mst operation at all. So no dp-aux or anything like that,
>>>     we just hotplug-create connectors with names and PATH property and MST
>>>     type, without any of the kernel-internal presentations for hubs/branch
>>>     points and all that stuff. Because userspace doesn't ever see that.
>>
>> I knew that user space don't really see the MST information (apart from
>> PATH), but I did not think about this solution. This may work well to test
>> user space, I agree!
>>
>> I think we are on the good track with José, he is trying to implement
>> connector hot-creation through ConfigFS [1]. To add "MST emulation", we can
>> "simply" add the PATH property through ConfigFS.
> 
> Yeah, I think that's the way to go. Well maybe with a change to always
> include the PATH property, because currently that's true for all
> hotpluggable connectors. And we probably want to keep that if we extend it
> to hotpluggable bridges or similar.
> 
>> [1]: Few discussions here
>> https://lore.kernel.org/all/Z5zJ1rEZyBEgd7DN@louis-chauvet-laptop/
> 
> Agreeing with you, connector hotplug is something we need to tackle as an
> extension of the basic configfs support, since it's quite complex.
> 
>>> - Next up is expressing all the funny constraints that can result in,
>>>     across different drivers. For that I think we want ebpf to implement the
>>>     various atomic_check hooks, so that you can implement all the funny
>>>     constraints of mst link bw limitations, but also host-side display
>>>     limitations. And I concur with Maxime that this ebpf support should be
>>>     entirely agnostic and just allow you to attach atomic_check
>>>     implementations to connectors, planes and crtcs. And then maybe one for
>>>     the overall commit, so that global constraints are easier to implement.
>>
>> If I understand correctly, this has nothing to do with VKMS + MST?
>> I don't clearly understand the use case: why do we want the user space to
>> express hardware limitations? If you have already discussed this on the
>> mailing list, can you share the discussion?
> 
> Not sure this was discussed in-depth, but when you get into more complex
> output configurations, there's all kinds of funny hw constraints that pop
> up. Examples:
> - Multiple mst outputs on the same physical port share the overall
>    bandwidth. So individually you might be able to light up each connector
>    at max resolution, but if you try to light up all of them, there's a
>    limitation. This is the mst specific case.
> - There's also lots of display hw constraints, like a limited amount of
>    clocks (fewer than crtc), or memory bw constraints for scanout, and
>    similar things.
> 
> The idea is to express these constraints with ebpf programs, so that you
> can validate that a compositor correctly handles these cases and doesn't
> try an invalide configuration and then just fails instead of trying to
> fall back to something that works.
> 
> So it's a much bigger issue, but multi-connector mst is a fairly important
> case here.

I think I understood. I don't think I will have the time to work on this 
issue, but it is clearly a good idea. I submitted [2] to add it on the 
TODO list.

[2]:https://lore.kernel.org/all/20250225-vkms-update-todo-v1-1-afb1e6f7d714@bootlin.com/ 
(not yet on lore)

Thanks,
Louis Chauvet


> Cheers, Sima
> 
>>> So summary: MST testing in vkms only needs to look like MST to userspace.
>>> Internally we'll just hand-roll the entire connector hotplug and leave out
>>> everything else. Because testing driver dp mst implementations and the
>>> helpers is better covered through different means.
>>>
>>> Eventually we might want to implement fake i2c and dp-aux implementations
>>> for additional features like TV remote control and fun stuff like that (I
>>> forgot the CEA/HDMI name for these). But I think that's waaaayyyyyy down
>>> the road.
>>
>> I think I am not that far from a full dp-aux emulation, it is precisely what
>> I tried to do in VKMS. I don't have the time to transform it to kunit tests.
>>
>> Thanks a lot for your feedback!
>> Louis Chauvet
>>
>>> Cheers, Sima
>>
>> -- 
>> Louis Chauvet, Bootlin
>> Embedded Linux and Kernel engineering
>> https://bootlin.com
>>
> 

-- 
Louis Chauvet, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com